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

@@ -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
 };

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_MD5, "MD5", &ssh_md5, 128, true)                      \
-    X(HTTP_DIGEST_SHA256, "SHA-256", &ssh_sha256, 256)                  \
+    X(HTTP_DIGEST_SHA256, "SHA-256", &ssh_sha256, 256, true)            \
-    X(HTTP_DIGEST_SHA512_256, "SHA-512-256", &ssh_sha512, 256)          \
+    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,

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))

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,

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",

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)