From 445f9de1297e408b9ca08f231873e312eb2222d4 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 10 Feb 2022 18:51:19 +0000 Subject: [PATCH 1/5] Fix handling of shifted SCO function keys. A user points out that this has regressed since 0.76, probably when I reorganised the keyboard control-sequence formatting into centralised helper functions in terminal.c. The SCO function keys should behave differently when you press Shift or Ctrl or both. For example, F1 should generate ESC[M bare, ESC[Y with Shift, Esc[k with Ctrl, Esc[w with Shift+Ctrl. But in fact, Shift was having no effect, so those tests would give ESC[M twice and ESC[k twice. That was because I was setting 'shift = false' for all function key types except FUNKY_XTERM_216, after modifying the derived 'index' value. But the SCO branch of the code doesn't use 'index' (it wouldn't have the right value in any case), so the sole effect was to forget about Shift. Easily fixed by disabling that branch for FUNKY_SCO too. (cherry picked from commit aa01530488e0b693172b513836fa6881d387ea8a) --- terminal/terminal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terminal/terminal.c b/terminal/terminal.c index 3398cd59..923ed03e 100644 --- a/terminal/terminal.c +++ b/terminal/terminal.c @@ -7373,7 +7373,7 @@ int format_function_key(char *buf, Terminal *term, int key_number, assert(key_number < lenof(key_number_to_tilde_code)); int index = key_number; - if (term->funky_type != FUNKY_XTERM_216) { + if (term->funky_type != FUNKY_XTERM_216 && term->funky_type != FUNKY_SCO) { if (shift && index <= 10) { shift = false; index += 10; From 5c9a43f47872a3c1eed99863fefda9b13a5f9e49 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 19 Feb 2022 11:55:07 +0000 Subject: [PATCH 2/5] HTTP proxy: support 'Transfer-encoding: chunked'. I had a report that the Windows free-as-in-beer proxy tool 'FreeProxy' didn't work with the new HTTP proxy code, and it turns out that the first reason why not is that the error-document in its 407 response is sent via chunked transfer encoding, which is to say, instead of an up-front Content-length header, you receive a sequence of chunks each prefixed with a hex length. (In 0.76, before the rewritten proxy support, we never even noticed this because we sent Basic auth details up front in our first attempt, rather than attempting a no-auth connection first and waiting to see what kind of auth the proxy asks us for. So we'd only ever see a 407 if the auth details were refused - and since 0.76 didn't have interactive proxy auth prompts, there was nothing we could do at that point but abort immediately, without attempting to parse the rest of the 407 at all.) Now we spot the Transfer-encoding header and successfully parse chunked transfers. Happily, we don't need to worry about the further transfer-encodings such as 'gzip', because we're not actually _using_ the error document - we only have to skip over it to find the end of the HTTP response. This still doesn't make PuTTY work with FreeProxy, because there are further problems hiding behind that one, which I'll fix in following commits. --- proxy/http.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 3 deletions(-) diff --git a/proxy/http.c b/proxy/http.c index 081fefb6..e01329ea 100644 --- a/proxy/http.c +++ b/proxy/http.c @@ -72,7 +72,8 @@ typedef struct HttpProxyNegotiator { uint32_t nonce_count; prompts_t *prompts; int username_prompt_index, password_prompt_index; - size_t content_length; + size_t content_length, chunk_length; + bool chunked_transfer; ProxyNegotiator pn; } HttpProxyNegotiator; @@ -151,6 +152,7 @@ static void proxy_http_free(ProxyNegotiator *pn) #define HTTP_HEADER_LIST(X) \ X(HDR_CONNECTION, "Connection") \ X(HDR_CONTENT_LENGTH, "Content-Length") \ + X(HDR_TRANSFER_ENCODING, "Transfer-Encoding") \ X(HDR_PROXY_AUTHENTICATE, "Proxy-Authenticate") \ /* end of list */ @@ -460,6 +462,7 @@ static void proxy_http_process_queue(ProxyNegotiator *pn) crReturnV; s->content_length = 0; + s->chunked_transfer = false; s->connection_close = false; /* @@ -530,6 +533,23 @@ static void proxy_http_process_queue(ProxyNegotiator *pn) if (!get_token(s)) continue; s->content_length = strtoumax(s->token->s, NULL, 10); + } else if (hdr == HDR_TRANSFER_ENCODING) { + /* + * The Transfer-Encoding header value should be a + * comma-separated list of keywords including + * "chunked", "deflate" and "gzip". We parse it in the + * most superficial way, by just looking for "chunked" + * and ignoring everything else. + * + * It's OK to do that because we're not actually + * _using_ the error document - we only have to skip + * over it to find the end of the HTTP response. So we + * don't care if it's gzipped or not. + */ + while (get_token(s)) { + if (!stricmp(s->token->s, "chunked")) + s->chunked_transfer = true; + } } else if (hdr == HDR_CONNECTION) { if (!get_token(s)) continue; @@ -584,8 +604,62 @@ static void proxy_http_process_queue(ProxyNegotiator *pn) } while (s->header->len > 0); /* Read and ignore the entire response document */ - crMaybeWaitUntilV(bufchain_try_consume( - pn->input, s->content_length)); + if (!s->chunked_transfer) { + /* Simple approach: read exactly Content-Length bytes */ + crMaybeWaitUntilV(bufchain_try_consume( + pn->input, s->content_length)); + } else { + /* Chunked transfer: read a sequence of + * \r\n\r\n chunks, terminating in one with + * zero length */ + do { + /* + * Expect a chunk length + */ + s->chunk_length = 0; + while (true) { + char c; + crMaybeWaitUntilV(bufchain_try_fetch_consume( + pn->input, &c, 1)); + if (c == '\r') { + continue; + } else if (c == '\n') { + break; + } else if ('0' <= c && c <= '9') { + s->chunk_length = s->chunk_length*16 + (c-'0'); + } else if ('A' <= c && c <= 'F') { + s->chunk_length = s->chunk_length*16 + (c-'A'+10); + } else if ('a' <= c && c <= 'f') { + s->chunk_length = s->chunk_length*16 + (c-'a'+10); + } else { + pn->error = dupprintf( + "Received bad character 0x%02X in chunk length " + "during HTTP chunked transfer encoding", + (unsigned)(unsigned char)c); + crStopV; + } + } + + /* + * Expect that many bytes of chunked data + */ + crMaybeWaitUntilV(bufchain_try_consume( + pn->input, s->chunk_length)); + + /* Now expect \r\n */ + { + char buf[2]; + crMaybeWaitUntilV(bufchain_try_fetch_consume( + pn->input, buf, 2)); + if (memcmp(buf, "\r\n", 2)) { + pn->error = dupprintf( + "Missing CRLF after chunk " + "during HTTP chunked transfer encoding"); + crStopV; + } + } + } while (s->chunk_length); + } if (200 <= s->http_status && s->http_status < 300) { /* Any 2xx HTTP response means we're done */ From 099d00c4acaaa28f660d3a941eaadbaa012b5924 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 19 Feb 2022 11:56:54 +0000 Subject: [PATCH 3/5] HTTP proxy: accept the 'Proxy-Connection' header. FreeProxy sends this as a substitute for the standard 'Connection' header (with the same contents, i.e. 'keep-alive' or 'close' depending on whether the TCP connection is going to continue afterwards). The Internet reckons it's not standard, but it's easy to recognise as an ad-hoc synonym for 'Connection'. --- proxy/http.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/proxy/http.c b/proxy/http.c index e01329ea..761abdf6 100644 --- a/proxy/http.c +++ b/proxy/http.c @@ -154,6 +154,7 @@ static void proxy_http_free(ProxyNegotiator *pn) X(HDR_CONTENT_LENGTH, "Content-Length") \ X(HDR_TRANSFER_ENCODING, "Transfer-Encoding") \ X(HDR_PROXY_AUTHENTICATE, "Proxy-Authenticate") \ + X(HDR_PROXY_CONNECTION, "Proxy-Connection") \ /* end of list */ typedef enum HttpHeader { @@ -550,7 +551,8 @@ static void proxy_http_process_queue(ProxyNegotiator *pn) if (!stricmp(s->token->s, "chunked")) s->chunked_transfer = true; } - } else if (hdr == HDR_CONNECTION) { + } else if (hdr == HDR_CONNECTION || + hdr == HDR_PROXY_CONNECTION) { if (!get_token(s)) continue; if (!stricmp(s->token->s, "close")) From 6c754822bca7284374f15ba90392e598909f9833 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 19 Feb 2022 12:04:09 +0000 Subject: [PATCH 4/5] Proxy system: ability to reconnect to the proxy server. Another awkward thing that FreeProxy does is to slam the connection shut after sending its 407 response, at least in Basic auth mode. (It keeps the connection alive in digest mode, which makes sense to me, because that's a more stateful system.) It was surprisingly easy to make the proxy code able to tolerate this! I've set it up so that a ProxyNegotiator can just set its 'reconnect' flag on return from the negotiation coroutine, and the effect will be that proxy.c makes a new connection to the same proxy server before doing anything else. In particular, you can set that flag _and_ put data in the output bufchain, and there's no problem - the output data will be queued directly into the new socket. --- proxy/http.c | 6 +++--- proxy/proxy.c | 25 +++++++++++++++++++++---- proxy/proxy.h | 10 ++++++++++ 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/proxy/http.c b/proxy/http.c index 761abdf6..d4797171 100644 --- a/proxy/http.c +++ b/proxy/http.c @@ -670,9 +670,9 @@ static void proxy_http_process_queue(ProxyNegotiator *pn) /* 407 is Proxy Authentication Required, which we may be * able to do something about. */ if (s->connection_close) { - pn->error = dupprintf("HTTP proxy closed connection after " - "asking for authentication"); - crStopV; + /* If we got 407 + connection closed, reconnect before + * sending our next request. */ + pn->reconnect = true; } /* If the best we can do is report some kind of error from diff --git a/proxy/proxy.c b/proxy/proxy.c index 36105097..6bedcf9b 100644 --- a/proxy/proxy.c +++ b/proxy/proxy.c @@ -99,6 +99,7 @@ static void sk_proxy_close (Socket *s) ProxySocket *ps = container_of(s, ProxySocket, sock); sk_close(ps->sub_socket); + sk_addr_free(ps->proxy_addr); sk_addr_free(ps->remote_addr); proxy_negotiator_cleanup(ps); bufchain_clear(&ps->output_from_negotiator); @@ -223,6 +224,16 @@ static void proxy_negotiate(ProxySocket *ps) return; } + if (ps->pn->reconnect) { + sk_close(ps->sub_socket); + SockAddr *proxy_addr = sk_addr_dup(ps->proxy_addr); + ps->sub_socket = sk_new(proxy_addr, ps->proxy_port, + ps->proxy_privport, ps->proxy_oobinline, + ps->proxy_nodelay, ps->proxy_keepalive, + &ps->plugimpl); + ps->pn->reconnect = false; + } + while (bufchain_size(&ps->output_from_negotiator)) { ptrlen data = bufchain_prefix(&ps->output_from_negotiator); sk_write(ps->sub_socket, data.ptr, data.len); @@ -601,10 +612,16 @@ Socket *new_connection(SockAddr *addr, const char *hostname, /* create the actual socket we will be using, * connected to our proxy server and port. */ - ps->sub_socket = sk_new(proxy_addr, - conf_get_int(conf, CONF_proxy_port), - privport, oobinline, - nodelay, keepalive, &ps->plugimpl); + ps->proxy_addr = sk_addr_dup(proxy_addr); + ps->proxy_port = conf_get_int(conf, CONF_proxy_port); + ps->proxy_privport = privport; + ps->proxy_oobinline = oobinline; + ps->proxy_nodelay = nodelay; + ps->proxy_keepalive = keepalive; + ps->sub_socket = sk_new(proxy_addr, ps->proxy_port, + ps->proxy_privport, ps->proxy_oobinline, + ps->proxy_nodelay, ps->proxy_keepalive, + &ps->plugimpl); if (sk_socket_error(ps->sub_socket) != NULL) return &ps->sock; diff --git a/proxy/proxy.h b/proxy/proxy.h index ca4a293f..72d06d49 100644 --- a/proxy/proxy.h +++ b/proxy/proxy.h @@ -22,6 +22,11 @@ struct ProxySocket { SockAddr *remote_addr; int remote_port; + /* Parameters needed to make further connections to the proxy */ + SockAddr *proxy_addr; + int proxy_port; + bool proxy_privport, proxy_oobinline, proxy_nodelay, proxy_keepalive; + bufchain pending_output_data; bufchain pending_oob_output_data; bufchain pending_input_data; @@ -68,6 +73,11 @@ struct ProxyNegotiator { /* Set to report user abort during proxy negotiation. */ bool aborted; + + /* Set to request the centralised code to make a fresh connection + * to the proxy server, e.g. because an HTTP proxy slammed the + * connection shut after sending 407 Proxy Auth Required. */ + bool reconnect; }; struct ProxyNegotiatorVT { From f85716be45a0cd47085c978744a6276fe189175e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 19 Feb 2022 12:06:29 +0000 Subject: [PATCH 5/5] HTTP proxy: accept Digest algorithm name as a quoted string. FreeProxy sends 'algorithm="MD5"' instead of 'algorithm=MD5.' I'm actually not sure whether that's legal by RFC 7616, but it's certainly no trouble to parse if we see it. With all these changes, PuTTY now _can_ successfully make connections through FreeProxy again, whether it's in Basic or Digest mode. --- proxy/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/http.c b/proxy/http.c index d4797171..0738e37d 100644 --- a/proxy/http.c +++ b/proxy/http.c @@ -325,7 +325,7 @@ static HttpAuthDetails *parse_http_auth_header(HttpProxyNegotiator *s) d->hash_username = !stricmp(s->token->s, "true"); } else if (!stricmp(s->token->s, "algorithm")) { if (!get_separator(s, '=') || - !get_token(s)) + (!get_token(s) && !get_quoted_string(s))) return auth_error(d, "parse error in Digest algorithm " "field"); bool found = false;