Open Bug 472823 Opened 13 years ago Updated 16 days ago

SHA 256 Digest Authentication

Categories

(Core :: Networking, enhancement, P5)

enhancement

Tracking

()

REOPENED

People

(Reporter: ph, Assigned: gs-bugzilla.mozilla.org, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [necko-would-take])

Attachments

(5 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121621 Ubuntu/8.04 (hardy) Firefox/3.0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121621 Ubuntu/8.04 (hardy) Firefox/3.0.5

Firefox implements Digest authentication using the MD5 algorithm, which is the example specified in RFC2617[1].  That RFC allows other algorithms, by using the optional 'algorithm' property:

   An optional header allows the server to specify the algorithm used to
   create the checksum or digest. By default the MD5 algorithm is used
   and that is the only algorithm described in this document.

Since MD5 has been cracked and is now considered vulnerable, I'd like to propose making Firefox recognize this header and implement other algorithms, at the least SHA-1 but later also SHA-2 variants like SHA-256 and SHA-512[2].

Obviously SSL provides a better security model but it is harder to use and fixing the Digest mechanism would give good benefits at little cost afaics.

RFC 2617 allows for a server to issue multiple challenges, using different algorithms, so this allows for negotiation between the browser and server.

[1] http://tools.ietf.org/html/rfc2617
[2] http://en.wikipedia.org/wiki/SHA_hash_functions


Reproducible: Always
Moving to Core->Networking and updating summary.  Not confirming because I actually don't know if we already support other algs for digest auth?
Component: Security → Networking
Product: Firefox → Core
QA Contact: firefox → networking
Summary: Digest Authentication is not secure (MD5 has been broken) → Digest Authentication should support other hash algorithms (MD5 has been broken)
Jason: if you're not otherwise assigned this bug might provide a path into learning about parts of necko that's somewhat more rewarding than just slogging through randomly.
Assignee: nobody → jduell
FWIW, if you need a web server to test against, we'll implement the SHA-1 algorithm in ours (Xitami/5).  Ask me, offlist.
Yes, this might be a good entryway into necko for me.  I've also just assigned myself to a half-dozen other networking bugs, so y'all can feel free to chime in on which ones ought to have highest priority ;)
So we definitely want to fix bug 281851 before this one--improved digest security won't matter if we use Basic auth instead whenever the web server lists it first.
There is a new RFC for Http Digest Authentication:
http://tools.ietf.org/html/rfc7616
This specification defines the following algorithms:
   o  SHA2-256 (mandatory to implement)
   o  SHA2-512/256 (as a backup algorithm)
   o  MD5 (for backward compatibility).
This patch adds support for SHA-256 as the digest algorithm.

My C++ skills are very unimpressive so i would advise someone with more skill to review and improve the code.

To test this code, you need a webserver with support for SHA-256.
I used a modified version of https://httpbin.org/ which can be found here:
https://github.com/Jaaap/httpbin (clone it, then `python setup.py develop` and then `python -m httpbin.core` should work)
we would want an ietf standard around this.. otherwise add-on territory.
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
(In reply to Patrick McManus [:mcmanus] from comment #8)
> we would want an ietf standard around this.. otherwise add-on territory.

Are you kidding? You've been given the standard, see comment #6, it is RFC 7616 from September 2015. http://tools.ietf.org/html/rfc7616.

So please reopen the BUG report. I implemented this into my web-services in less than half an hour, and now I am looking for web browsers supporting it. Firefox could be the first one ;-)
This seems to have been closed in error. RFC 7616 is an IETF standard. The reason stated for closing is bogus.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
(In reply to Dr. Rolf Jansen from comment #9)
.
> 
> Are you kidding? You've been given the standard, see comment #6, it is RFC
> 7616 from September 2015. http://tools.ietf.org/html/rfc7616.
>

I missed this. Sorry. And the patch wasn't on anyone's radar because it wasn't flagged for review - it was just another attachment. Thanks doubly for the patch.

I'm still not sure if we want this code if there is no interest from other singificant entities on the web.. that's always a tricky thing - sometimes you need to solve the chicken and egg problem, and other times you want to avoid propogating too many 'standards'.

Dan might be in the best place to decide.
Flags: needinfo?(dveditz)
Whiteboard: [necko-would-take]
Attachment #8683095 - Flags: review?(dveditz)
Summary: Digest Authentication should support other hash algorithms (MD5 has been broken) → SHA 256 Digest Authentication
I don't know why we wouldn't want to implement this. MD5 is known crap and this is an approved standard that appears to have been reviewed by the right smart folks (including our own :bagder).
Flags: needinfo?(dveditz)
the reason we wouldn't want to do this is that there is very little interest in digest authentication even with this standard existing and its tough to maintain code or tests without a champion for them.

I'm not saying that's a winning argument - just an argument. If you want to review the code and think its worth including I'm good with that.
The reason why the "old kind of" digest authentication got very little interest nowadays is exactly because of the weakness of MD5, and because everybody has been told to go with TLS anyway so the authentication can be as simple as asking for a password on the web site and generate a session cookie. ****, simply ask the big five suppliers of firewall appliances how many DPI systems with TLS interception they deploy per year. How many people enter their private web-mailer from work, and don't even know that their login credentials would be visible to a dedicated company admin. How many people open the day or the other their WordPress Blog from behind a DPI-FW? The most critical part of this web-traffic are the credentials and these would be safe by SHA256-Digest-Authentication over TLS.

Another advantage of digest authentication compared to other web site authentication methods is, that the credentials are passed in the HTTP message header and not in the HTTP message body (no to be confused with HTML header/body). With digest authentication you could safely utilize HTTP message body compression without having fear about the "BREACH" family of attacks.

That there would be no interest on a better digest authentication with SHA256/SHA512 algorithms can only be very poor assumption of yours, given that yourself learned about this new standard only a few days ago.
I support implementing RFC7616 as well. I've used older HTTP Digest and having the new one work would be excellent.

I've used Digest plenty times before and appreciate the fact that I do not need to come up with my own password hashing scheme (already provided) and don't have to bother to get an SSL set up right and then keep it up to date. For plenty of use cases Digest is perfect.

Thank you!
Hi all,
I waited for this feature for a very long time, hoping that someone should catch on that RFC7616 is no longer optional but critical.
This is not only about the browser but can increase server side security in the case of database stored digested HA1s.
APIs, modems, routers, js based auth clients, all can benefit from this if used corectly together with TLS.

In the era of GPU computing, MD5 is not only just obsolete, but unsecure as well.

HTTP Digest with only MD5 prevents the use of the strong password encryption, meaning that the HA1 strings stored on the server are in the reach of a single machine with enough GPU power.

I don't know how to raise this bug awareness, but this will become critical sooner than you guys might think.

Hopefully someone watches this and Firefox 56 will have support for RFC7616..
Thank you.
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Comment on attachment 8683095 [details] [diff] [review]
http-digest-sha256.patch

Review of attachment 8683095 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz

::: netwerk/protocol/http/nsHttpDigestAuth.cpp
@@ +60,5 @@
> +  uint32_t hlen = 16;
> +  if (algorithm & ALGO_SHA256)
> +  {
> +    rv = mVerifier->Init(nsICryptoHash::SHA256);
> +    hlen = 32;

Would prefer to see names for these numbers and ones introduced later in this patch.

::: netwerk/protocol/http/nsHttpDigestAuth.h
@@ +88,5 @@
>                                  nsACString & aHeaderLine);
>  
>    protected:
>      nsCOMPtr<nsICryptoHash>        mVerifier;
> +    char                           mHashBuf[32];

could you use EXPANDED_DIGEST_LENGTH or create a #define? It looks like this code is trying to avoid magic numbers.
Attachment #8683095 - Flags: review?(dveditz) → review+

If someone wants to rebase the patch, and is willing to add unit tests for it in netwerk/test/unit/test_authentication.js it would be great to land it.

Assignee: jduell.mcbugs → nobody

Guys... there is a very common use-case for home routers where they want to transfer authentication without tls.

Making them transfer it in MD5 because the privacy-aware and security-aware browser doesn't implement SHA-256 for this is insecure.

Chrome is also broken for this (ignores algorithm= and continues with md5) but if you think about your own home router... it's a missing piece of the security puzzle isn't it?

I'm currently adding support for Digest auth to a web server, but I'm hitting the wall of SHA-256 being unsupported by all major web browsers. One can only wonder: why? If Digest auth is already supported, adding support for more secure algorithms seems like a minor inconvenience, just a few lines of code away.

It may be of little use today, but it has clear advantages over an HTML form. It requires no CGI, the web server is free to manage sessions at will, and it provides fairly acceptable security without TLS. And if you need absolute security, Digest+TLS with a strong hashing algorithm is thw way to go. TLS makes sure no third party messes around with our connection, and Digest makes sure we don't send our password as cleartext over the network. Imagine the security breaches that could have been avoided if we just followed the standard, secure-by-default path, so web admins had to confront password hashing as an immediate priority, and not just as an afterthought: no, you are not storing plaintext passwords anywhere, because you are not getting them in the first place. By default.

I really think that Digest+TLS+a better UI for standard authentication (one more pleasing, which shows info about the host, the realm, and the algorithm being used) may be not only attractive for web developers, but also desirable (and, depending on the environment—routers, intranets and such, also neccesary). And I may start working on that last one if this goes anywhere.

(In reply to Valentin Gosu [:valentin] (he/him) from comment #20)

If someone wants to rebase the patch, and is willing to add unit tests for it in netwerk/test/unit/test_authentication.js it would be great to land it.

I've created new patch based on Teun van Eijsden's with proposed improvements. I'd be happy if someone with proper rights reviews it and runs the tests.

(In reply to Vit Hampl from comment #23)

I've created new patch based on Teun van Eijsden's with proposed improvements. I'd be happy if someone with proper rights reviews it and runs the tests.

I'm not a C++ developer, but from what i can tell, it looks good.

I also filed an issue over at the chromium project and apple-support (no link cause of policy stuff) for supporting rfc7616 that are currently awaiting triage, so if all goes well, we might see proper support for this across the board soon.

Glad to see this is still alive. A long time ago, I had rewritten the original patch from Teun van Eijsden, but did not have sufficient disk space on my laptop to build Firefox. Now that I do, I have tested my patches against lighttpd, which has supported algorithm=SHA-256 since lighttpd 1.4.54 (released May 2019) (https://redmine.lighttpd.net/projects/lighttpd/wiki/Release-1_4_54). I'll attach my patches shortly.

My patches also improve preferential ordering for algorithm selection. (Any privileged committer: please let me know if I should submit that patch separately.) https://bugzilla.mozilla.org/show_bug.cgi?id=281851

My patches differs slightly from those of Vit Hampl, though I have incorporated the tests that Vit added (thanks!) However Vit's test has two errors.

algorithmRE must match "SHA-256", but \w+ does not match '-'
fix: var algorithmRE = /algorithm=([\w-]+)/;

A2 must include the url-path. This was copied from the MD5 test (with test url-path /auth/digest), but not updated to /auth/digest_sha
fix: var A2 = "GET:/auth/digest_sha";

Tests have been run with ./mach test netwerk/test/unit/test_authentication on x86_64 debug build.

Bug 472823 SHA 256 Digest Authentication
Original patch by Teun van Eijsden
Tests added by Vit Hampl <mozilla@bugear.com>
Original patch updated and tests fixed by gstrauss

Bug 281851 CVE-2005-2395 Wrong scheme used when server offers both Basic and Digest auth [rfc2617 obsoletes rfc2068]

At the risk of submitting the patch twice, I'll try submitting with moz-phab next.

fixes:
Bug 472823 SHA 256 Digest Authentication
Original patch by Teun van Eijsden
Tests added by Vit Hampl <mozilla@bugear.com>
Original patch updated and tests fixed by gstrauss

fixes:
Bug 281851 CVE-2005-2395 Wrong scheme used when server offers both Basic and Digest auth

Assignee: nobody → gs-bugzilla.mozilla.org

Vit, should I add support for "SHA-256-sess"? Your patch recognizes it, my patch does not.

Does any popular server implement "SHA-256-sess"? Does any popular server implement "MD5-sess"?
Apache does not implement MD5-sess (https://httpd.apache.org/docs/trunk/mod/mod_auth_digest.html#authdigestalgorithm)
(From a quick glance at the doc, Apache and nginx do not appear to support "SHA-256" digest auth)

Nice to see a progress. As for the -sess variant, from my point of view I don't see any strong reason not to add it. It is in the standard and it does not mean adding excessive amount of code, as it just utilises existing mechanism only with different hashing algorithm. There is bug https://bugzilla.mozilla.org/show_bug.cgi?id=270447 in the existing mechanism that limits the current use, but fix of the bug will be algorithm-independent.
From looking around, IIS should support MD5-sess. I'm not aware of any popular server currently supporting SHA-256-sess.

As for the -sess variant, from my point of view I don't see any strong reason not to add it. It is in the standard and it does not mean adding excessive amount of code

I agree that it probably does not add much code. However, it should ideally also have a unit test. Do you have interest in adding one?

I tested my version of the SHA-256 patches against a real webserver (lighttpd) in addition to the unit test. I am familiar with no such web server to be able to test SHA-256-sess and am hesitant to add support that I would not be confident testing. ...Update: someone noted that passport-http supports MD5-sess in https://github.com/postmanlabs/postman-app-support/issues/2941 but I have not verified that.

==> If the patches proposed here are accepted, adding support for additional algorithms, such as SHA-256-sess or SHA-512-256, becomes easier once there is a demonstrated use/need.

I updated the proposed patch to include "SHA-256-sess" and to prefer SHA-256 when multiple challenges with both SHA-256 and MD5 are offered.
https://phabricator.services.mozilla.com/D106241

Attached file unit-parts.js
The unit test for -sess variant shoud only differ in A1 computation (and parts ). 
```javascript
      var A1;
      if (algorithm != null && algorithm.endsWith("-sess")){
        A1 = [HXXX("guest:secret:guest"), nonce, cnonce].join(":"); //replace HXXX with actual hashing function
      } else {
        A1 = "guest:secret:guest";
      }
```
Attached file unit-parts.js (obsolete) —
The unit test for -sess variant shoud only differ in A1 computation (and parts ). 
```javascript
      var A1;
      if (algorithm != null && algorithm.endsWith("-sess")){
        A1 = [HXXX("guest:secret:guest"), nonce, cnonce].join(":"); //replace HXXX with actual hashing function
      } else {
        A1 = "guest:secret:guest";
      }
```
Attached file unit-parts.txt (obsolete) —
The unit test for -sess variant shoud only differ in A1 computation (and parts ). 
```javascript
      var A1;
      if (algorithm != null && algorithm.endsWith("-sess")){
        A1 = [HXXX("guest:secret:guest"), nonce, cnonce].join(":"); //replace HXXX with actual hashing function
      } else {
        A1 = "guest:secret:guest";
      }
```
Attached file unit-parts.js (obsolete) —
Attachment #9206510 - Attachment is obsolete: true
Attachment #9206511 - Attachment is obsolete: true
Attachment #9206509 - Attachment is obsolete: true

The unit test for -sess variants should only differ in A1 computation (and things around like headers). More extensive examples are in attachment unit-parts.js (sorry for probable spamming with attachments, it always showed an error and only after several attempts I found out, that they all got uploaded).

      var A1;
      if (algorithm != null && algorithm.endsWith("-sess")){
        A1 = [HXXX("guest:secret:guest"), nonce, cnonce].join(":"); //replace HXXX with actual hashing function
      } else {
        A1 = "guest:secret:guest";
      }

Thanks @Vit. There were a few mistakes in your attachment which I fixed up and added to https://phabricator.services.mozilla.com/D106241

Attachment #9205037 - Attachment description: Bug 472823 - support SHA-256 HTTP Digest auth → Bug 472823 - support SHA-256 HTTP Digest auth r=#necko
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3259a8cb3db1
support SHA-256 HTTP Digest auth r=necko-reviewers,dragana

Backed out 14 changesets (Bug 1705659, Bug 472823, Bug 669675) for causing bustages in nsHttpChannelAuthProvider.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/a9431e068bcd2dc86778d9b12bc7775850e2736b
Push with failures, failure log.

(Update): Also caused mochitest plain failures.

Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/27e709923ecb
support SHA-256 HTTP Digest auth r=necko-reviewers,dragana
Status: REOPENED → RESOLVED
Closed: 5 years ago21 days ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Duplicate of this bug: httpauthorder

Backed out 14 changesets (Bug 1705659, Bug 472823, Bug 669675) as requested by valentin for causing regressions.
https://hg.mozilla.org/integration/autoland/rev/4207821cb4e016141be3c4b331f1ede47c90c0b7

Status: RESOLVED → REOPENED
Flags: needinfo?(valentin.gosu)
Resolution: FIXED → ---
Target Milestone: 90 Branch → ---
You need to log in before you can comment on or make changes to this bug.