net: a weak DH key is not a certificate error.

net: a weak DH key is not a certificate error.

BUG=348987
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254990

[Ryan Sleevi, 2014-03-04 19:28:34]

lgtm

[Ryan Sleevi, 2014-03-04 19:29:03]

Also, any chance of a unit test coming out of this? Does tlslite do EDH
exchanges, or is it only RSA (ISTR only RSA)

[wtc, 2014-03-04 20:13:30]

Patch set 1 LGTM. I suggest an additional change to
net/cert/cert_status_flags.{h,cc} for CERT_STATUS_WEAK_DH_KEY. We can also
optionally rename ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN and move it to the
certificate error range.

https://codereview.chromium.org/178003011/diff/1/net/base/net_errors.h
File net/base/net_errors.h (right):

https://codereview.chromium.org/178003011/diff/1/net/base/net_errors.h#newcode38
net/base/net_errors.h:38: // (inclusive) to net::ERR_CERT_END (exclusive) in
*decreasing* order.

This comment should be updated to note ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN as
an exception to this rule.

The rule is that all certificate errors should be within this range, and have a
corresponding certificate error status bit defined in cert_status_flags.h.

There is another related problem (also coming from
https://codereview.chromium.org/23908005/): ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY
has a corresponding CERT_STATUS_WEAK_DH_KEY bit. That certificate error status
bit should be removed.

As for ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN, we usually don't change the numeric
value of an error code. We can, however, replace it with a new error code
ERR_CERT_PINNED_KEY_MISSING within the certificate error range.

[agl, 2014-03-04 22:54:38]

https://codereview.chromium.org/178003011/diff/1/net/base/net_errors.h
File net/base/net_errors.h (right):

https://codereview.chromium.org/178003011/diff/1/net/base/net_errors.h#newcode38
net/base/net_errors.h:38: // (inclusive) to net::ERR_CERT_END (exclusive) in
*decreasing* order.
On 2014/03/04 20:13:30, wtc wrote:
> This comment should be updated to note ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN as
> an exception to this rule.

Done.

> The rule is that all certificate errors should be within this range, and have
a
> corresponding certificate error status bit defined in cert_status_flags.h.

Yea. The problem stems from the fact that I didn't intend for pinning to be a
certificate error. I planned on it being a simple, fatal error like a connection
failure.

> There is another related problem (also coming from
> https://codereview.chromium.org/23908005/%29:
ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY
> has a corresponding CERT_STATUS_WEAK_DH_KEY bit. That certificate error status
> bit should be removed.

Since this is a crash fix and will probably be merged, I'll keep it simple.

> As for ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN, we usually don't change the
numeric
> value of an error code. We can, however, replace it with a new error code
> ERR_CERT_PINNED_KEY_MISSING within the certificate error range.

Again, I'll avoid doing this in this change. I'd like to talk to felt about the
treatment of pinning errors first.

[agl, 2014-03-04 22:54:46]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-03-04 22:55:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/178003011/20001

[agl, 2014-03-04 23:10:04]

On Tue, Mar 4, 2014 at 2:29 PM,  <rsleevi@chromium.org> wrote:
> Also, any chance of a unit test coming out of this? Does tlslite do EDH
> exchanges, or is it only RSA (ISTR only RSA)

I'd love to write a version of the Go TLS stack with options for
breaking every possible thing. (I don't think TLSLite is going to
stretch to that.)

However, I don't want to let my current project out of my sight at the
moment (for obvious reasons) so it's on the eternal TODO for now.


Cheers

AGL

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-03-05 01:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/178003011/20001

[commit-bot: I haz the power, 2014-03-05 10:02:07]

Change committed as 254990