From 44055cd36ef0ee7cf9b03d3ff6ec39e51bfa92b6 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 27 Nov 2021 11:41:00 +0000 Subject: [PATCH] Withdraw support for SHA-512-256 in HTTP Digest. I was dubious about it to begin with, when I found that RFC 7616's example seemed to be treating it as a 256-bit truncation of SHA-512, and not the thing FIPS 180-4 section 6.7 specifies as "SHA-512/256" (which also changes the initial hash state). Having failed to get a clarifying response from the RFC authors, I had the idea this morning of testing other HTTP clients to see what _they_ thought that hash function meant, and then at least I could go with an existing in-practice consensus. There is no in-practice consensus. Firefox doesn't support that algorithm at all (but they do support SHA-256); wget doesn't support anything that RFC 7616 added to the original RFC 2617. But the prize for weirdness goes to curl, which does accept the name "SHA-512-256" and ... treats it as an alias for SHA-256! So I think the situation among real clients is too confusing to even try to work with, and I'm going to stop adding to it. PuTTY will follow Firefox's policy: if a proxy server asks for SHA-256 digests we'll happily provide them, but if they ask for SHA-512-256 we'll refuse on the grounds that it's not clear enough what it means. --- proxy/cproxy.c | 12 +++++-- proxy/cproxy.h | 79 ++++++++++++++++++++++++++++++++++++++++--- proxy/http.c | 17 ++++++++-- proxy/nocproxy.c | 4 ++- test/cryptsuite.py | 8 +++++ test/testcrypt-enum.h | 2 +- 6 files changed, 111 insertions(+), 11 deletions(-) diff --git a/proxy/cproxy.c b/proxy/cproxy.c index 29eafdfc..38e6eef9 100644 --- a/proxy/cproxy.c +++ b/proxy/cproxy.c @@ -41,18 +41,24 @@ void BinarySink_put_hex_data(BinarySink *bs, const void *vptr, size_t len) BinarySink_put_hex_data(BinarySink_UPCAST(bs), p, len) const char *const httphashnames[] = { - #define DECL_ARRAY(id, str, alg, bits) str, + #define DECL_ARRAY(id, str, alg, bits, accepted) str, + HTTP_DIGEST_HASHES(DECL_ARRAY) + #undef DECL_ARRAY +}; + +const bool httphashaccepted[] = { + #define DECL_ARRAY(id, str, alg, bits, accepted) accepted, HTTP_DIGEST_HASHES(DECL_ARRAY) #undef DECL_ARRAY }; static const ssh_hashalg *const httphashalgs[] = { - #define DECL_ARRAY(id, str, alg, bits) alg, + #define DECL_ARRAY(id, str, alg, bits, accepted) alg, HTTP_DIGEST_HASHES(DECL_ARRAY) #undef DECL_ARRAY }; static const size_t httphashlengths[] = { - #define DECL_ARRAY(id, str, alg, bits) bits/8, + #define DECL_ARRAY(id, str, alg, bits, accepted) bits/8, HTTP_DIGEST_HASHES(DECL_ARRAY) #undef DECL_ARRAY }; diff --git a/proxy/cproxy.h b/proxy/cproxy.h index c3e7ba15..4179895e 100644 --- a/proxy/cproxy.h +++ b/proxy/cproxy.h @@ -7,18 +7,89 @@ extern const bool socks5_chap_available; strbuf *chap_response(ptrlen challenge, ptrlen password); extern const bool http_digest_available; +/* + * List macro for the various hash functions defined for HTTP Digest. + * + * Of these, MD5 is the original one; SHA-256 is unambiguous; but + * SHA-512-256 seems to be controversial. + * + * RFC 7616 doesn't provide a normative reference, or any text + * explaining what they mean by it. They apparently expect you to + * already know. The problem with that is that there are two plausible + * things they _might_ have meant: + * + * 1. Ordinary SHA-512, truncated to 256 bits by discarding the + * second half of the hash output, per FIPS 180-4 section 7 (which + * says that in general it's OK to truncate hash functions like + * that if you need to). FIPS 180-4 assigns no particular specific + * spelling to this kind of truncated hash. + * + * 2. The same except that the initial state of the SHA-512 algorithm + * is reset to a different 512-bit vector to ensure that it's a + * distinguishable hash function in its own right, per FIPS 180-4 + * section 6.7 (which in turn refers to section 5.3.6.2 for the + * actual initial values). FIPS 180-4 spells this "SHA-512/256". + * + * The text of RFC 7616 is totally silent as to which of these they + * meant. Their spelling is inconsistent: the protocol identifier is + * "SHA-512-256", but in some places in the RFC they say + * "SHA-512/256", matching FIPS's spelling for the hash in option 2 + * above. On the other hand, the example authentication exchange in + * section 3.9.2 of the RFC contains hashes that are consistent with + * option 1 above (a truncation of plain SHA-512). + * + * Erratum 4897, https://www.rfc-editor.org/errata/eid4897, points out + * this ambiguity, and suggests correcting the example exchange to be + * consistent with option 2. However, as of 2021-11-27, that erratum + * is shown on the RFC Editor website in state "Reported", with no + * response (positive _or_ negative) from the RFC authors or anyone + * else. (And it was reported in 2016, so it's not as if they haven't + * had time.) + * + * So, which hash should we implement? Perhaps there's a consensus + * among existing implementations in the wild? + * + * I rigged up an HTTP server to present a SHA-512-256 Digest auth + * request, and tried various HTTP clients against it. The only HTTP + * client I found that accepts 'algorithm="SHA-512-256"' and sends + * back an auth attempt quoting the same hash is curl - and curl, + * bizarrely, seems to treat "SHA-512-256" as _neither_ of the above + * options, but as simply an alias for SHA-256! + * + * Therefore, I think the only safe answer is to refuse to support + * that hash at all: it's too confusing. + * + * However, I keep it in the list of hashes here, so that we can check + * the test case from RFC 7616, because that test case is also the + * only test of username hashing. So we reject it in proxy/http.c, but + * accept it in the internal function http_digest_response(), and + * treat it as option 1 (truncated SHA-512). + * + * Therefore, the parameters to each invocation of X in the following + * list macro are: + * + * - internal enum id for the hash + * - protocol identifier string + * - algorithm to use for computing it (as a const ssh_hashalg *) + * - length to truncate the output to + * - whether we accept it in http.c or not. + */ #define HTTP_DIGEST_HASHES(X) \ - X(HTTP_DIGEST_MD5, "MD5", &ssh_md5, 128) \ - X(HTTP_DIGEST_SHA256, "SHA-256", &ssh_sha256, 256) \ - X(HTTP_DIGEST_SHA512_256, "SHA-512-256", &ssh_sha512, 256) \ + X(HTTP_DIGEST_MD5, "MD5", &ssh_md5, 128, true) \ + X(HTTP_DIGEST_SHA256, "SHA-256", &ssh_sha256, 256, true) \ + X(HTTP_DIGEST_SHA512_256, "SHA-512-256", &ssh_sha512, 256, false) \ /* end of list */ + typedef enum HttpDigestHash { - #define DECL_ENUM(id, str, alg, bits) id, + #define DECL_ENUM(id, str, alg, bits, accepted) id, HTTP_DIGEST_HASHES(DECL_ENUM) #undef DECL_ENUM N_HTTP_DIGEST_HASHES } HttpDigestHash; + extern const char *const httphashnames[]; +extern const bool httphashaccepted[]; + void http_digest_response(BinarySink *bs, ptrlen username, ptrlen password, ptrlen realm, ptrlen method, ptrlen uri, ptrlen qop, ptrlen nonce, ptrlen opaque, uint32_t nonce_count, diff --git a/proxy/http.c b/proxy/http.c index 5176027c..bcedacc7 100644 --- a/proxy/http.c +++ b/proxy/http.c @@ -430,22 +430,35 @@ static void proxy_http_process_queue(ProxyNegotiator *pn) !get_token(s)) goto bad_digest; bool found = false; + size_t i; - for (size_t i = 0; i < N_HTTP_DIGEST_HASHES; i++) { + for (i = 0; i < N_HTTP_DIGEST_HASHES; i++) { if (!stricmp(s->token->s, httphashnames[i])) { - s->digest_hash = i; found = true; break; } } if (!found) { + /* We don't even recognise the name */ + pn->error = dupprintf( + "HTTP proxy requested Digest hash " + "algorithm '%s' which we do not recognise", + s->token->s); + crStopV; + } + + if (!httphashaccepted[i]) { + /* We do recognise the name but we + * don't like it (see comment in cproxy.h) */ pn->error = dupprintf( "HTTP proxy requested Digest hash " "algorithm '%s' which we do not support", s->token->s); crStopV; } + + s->digest_hash = i; } else if (!stricmp(s->token->s, "qop")) { if (!get_separator(s, '=') || !get_quoted_string(s)) diff --git a/proxy/nocproxy.c b/proxy/nocproxy.c index 38d416d5..89341489 100644 --- a/proxy/nocproxy.c +++ b/proxy/nocproxy.c @@ -20,7 +20,9 @@ strbuf *chap_response(ptrlen challenge, ptrlen password) unreachable("CHAP is not built into this binary"); } -const char *const httphashnames[] = { NULL }; /* dummy to prevent link error */ +/* dummy arrays to prevent link error */ +const char *const httphashnames[] = { NULL }; +const bool httphashaccepted[] = { false }; void http_digest_response(BinarySink *bs, ptrlen username, ptrlen password, ptrlen realm, ptrlen method, ptrlen uri, ptrlen qop, diff --git a/test/cryptsuite.py b/test/cryptsuite.py index a501bc27..e331bcf6 100755 --- a/test/cryptsuite.py +++ b/test/cryptsuite.py @@ -3212,6 +3212,14 @@ class standard_test_vectors(MyTestBase): # that they think it's just a 256-bit truncation of SHA-512, # and not the version defined in FIPS 180-4 which also uses # a different initial hash state), and username hashing. + # + # We don't actually support SHA-512-256 in the top-level proxy + # client code (see the comment in proxy/cproxy.h). However, + # this internal http_digest_response function still provides + # it, simply so that we can run this test case from the RFC, + # because it's the only provided test case for username + # hashing, and this confirms that we've got the preimage right + # for the username hash. params = ["J\u00E4s\u00F8n Doe".encode("UTF-8"), "Secret, or not?", "api@example.org", "GET", "/doe.json", "auth", diff --git a/test/testcrypt-enum.h b/test/testcrypt-enum.h index cec8d836..cf8f00c5 100644 --- a/test/testcrypt-enum.h +++ b/test/testcrypt-enum.h @@ -138,7 +138,7 @@ END_ENUM_TYPE(fptype) * invent a separate one for testcrypt, reuse the existing names. */ BEGIN_ENUM_TYPE(httpdigesthash) - #define DECL_ARRAY(id, str, alg, bits) ENUM_VALUE(str, id) + #define DECL_ARRAY(id, str, alg, bits, accepted) ENUM_VALUE(str, id) HTTP_DIGEST_HASHES(DECL_ARRAY) #undef DECL_ARRAY END_ENUM_TYPE(httpdigesthash)