<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi J-B,</p>
<p>On 2017-03-03 16:18, Jean-Baptiste Kempf wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> I don't like this patch too much, tbh.
IMHO, you should do a different if and a different message inside the
0x02 case, else, it's undebuggable for the user.</code></pre>
</blockquote>
<p>I do understand the initial thought about that the diagnostic should maybe not be shared between the cases, but with the current implementation (and the patch in which this email is about) the logs would contain an call to <code>msg_Err</code> stating <em>“socks: authentication needed”</em> (given that <code>b_auth = false</code> for the path in question) which I initially thought was enough.</p>
<p>As also pointed out by <em>courmisch</em> in <code>#videolan</code>, tokens that are shorter than <code>1</code> character are not acceptable as <em>username/password</em> in the <a href="https://tools.ietf.org/html/rfc1929#section-2"><em>SOCKS</em> specification</a>:</p>
<pre><code>This begins with the client producing a Username/Password request:
+----+------+----------+------+----------+
|VER | ULEN | UNAME | PLEN | PASSWD |
+----+------+----------+------+----------+
| 1 | 1 | 1 to 255 | 1 | 1 to 255 |
+----+------+----------+------+----------+</code></pre>
<p>See attached patch for one that tries to elaborate the diagnostic by stating, more explicitly, what is going on; I hope that this is enough as it would make me rest easier knowing that at least one potential null-dereference is gone from the codebase.</p>
<p>Thanks,<br />
Filip Roséen</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> On Wed, 1 Mar 2017, at 09:52, Filip Roséen wrote:</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> The previous implementation assumed that the remote entity would not
ask for authentication unless we have stated that we have the
capability for such, we should however not trust this assumption given
that it would (if it happens) cause problems with the username /
password being NULL.
---
src/network/tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/tcp.c b/src/network/tcp.c
index 4f4d4701f6..b85852de22 100644
--- a/src/network/tcp.c
+++ b/src/network/tcp.c
@@ -359,7 +359,7 @@ static int SocksNegotiate( vlc_object_t *p_obj,
{
msg_Dbg( p_obj, "socks: no authentication required" );
}
- else if( buffer[1] == 0x02 )
+ else if( buffer[1] == 0x02 && psz_socks_user && psz_socks_passwd )
{
int i_len1 = __MIN( strlen(psz_socks_user), 255 );
int i_len2 = __MIN( strlen(psz_socks_passwd), 255 );
--
2.12.0</code></pre>
</blockquote>
</blockquote>
</body>
</html>