1873263 - Webtransport: serverCertificateHashes does not work as expected

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[marten.richter]

The patch submitted to bug 1806693 and already merged does not work as expected (sorry for opening a new bug, but I could not comment on the closed bug). (But I really, really appreciate the work; I was waiting for this!)

I tried the serverCertificateHashes today with a working setup that works for Chromium with serverCertificateHashes.
Per spec, serverCertificateHashes should replace the certificate check in order to support self-signed certificates from e.g., automatically spanned VMs. (see https://www.w3.org/TR/webtransport/#certificate-hashes).

I used a self-signed certificate today, and I also built Firefox locally to spin up a debugging session.
My test code never hit "bool WebTransportSessionProxy::CheckServerCertificateIfNeeded() ".
I could track it down in pkixbuild.cpp in BuildForward. It does not pass the code right after PathBuildingStep.
In detail, it fails inside the PathBuildingStep::Check in this clause:

    if (InputsAreEqual(potentialIssuer.GetSubjectPublicKeyInfo(),
                       prev->GetSubjectPublicKeyInfo()) &&
        InputsAreEqual(potentialIssuer.GetSubject(), prev->GetSubject())) {
      // XXX: error code
      return RecordResult(Result::ERROR_UNKNOWN_ISSUER, keepGoing);
    }

where it complains about the self-signed certificate.

If you look closely at the patch, it successfully implements the passing of the serverCertificateHashes to the network library and introduces an additional check for serverCertificateHashes. However, the certificate check connected to the http3 connection is never turned off/replaced (or, better, conditionally turned off/replaced). This is what I can see in the debugger.

If it would be helpful, I can point you to a node.js code to generate such certificates. Here is one example (javascript syntax):

pem {
  private: '-----BEGIN PRIVATE KEY-----\r\n' +
    'MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgWK5oR/2weR+r9gs9\r\n' +
    'iopiMqQ9m4Wq5jOB0CmqIP0kuB6hRANCAAQmKuOf4R0HPVpqVP0iR6h2ZQcfUEmc\r\n' +
    '6Qhh0QWAwDRLk1/CEr5hdhfHgSaJUZONsTtz8X7H0YNsSFLQXtVA9G2S\r\n' +
    '-----END PRIVATE KEY-----\r\n',
  public: '-----BEGIN PUBLIC KEY-----\r\n' +
    'MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEJirjn+EdBz1aalT9IkeodmUHH1BJ\r\n' +
    'nOkIYdEFgMA0S5NfwhK+YXYXx4EmiVGTjbE7c/F+x9GDbEhS0F7VQPRtkg==\r\n' +
    '-----END PUBLIC KEY-----\r\n',
  cert: '-----BEGIN CERTIFICATE-----\r\n' +
    'MIIB1jCCAa2gAwIBAgIJDAYaZHzvuBIUMAwGCCqGSM49BAMCBQAwZjELMAkGA1UE\r\n' +
    'BhMCREUxDzANBgNVBAgTBkJlcmxpbjEPMA0GA1UEBxMGQmVybGluMSEwHwYDVQQK\r\n' +
    'ExhXZWJUcmFuc3BvcnQgVGVzdCBTZXJ2ZXIxEjAQBgNVBAMTCTEyNy4wLjAuMTAe\r\n' +
    'Fw0yNDAxMDUxODUzMDNaFw0yNDAxMTgxODUzMDNaMGYxCzAJBgNVBAYTAkRFMQ8w\r\n' +
    'DQYDVQQIEwZCZXJsaW4xDzANBgNVBAcTBkJlcmxpbjEhMB8GA1UEChMYV2ViVHJh\r\n' +
    'bnNwb3J0IFRlc3QgU2VydmVyMRIwEAYDVQQDEwkxMjcuMC4wLjEwWTATBgcqhkjO\r\n' +
    'PQIBBggqhkjOPQMBBwNCAAQmKuOf4R0HPVpqVP0iR6h2ZQcfUEmc6Qhh0QWAwDRL\r\n' +
    'k1/CEr5hdhfHgSaJUZONsTtz8X7H0YNsSFLQXtVA9G2So0IwQDAJBgNVHRMEAjAA\r\n' +
    'MAsGA1UdDwQEAwIC9DAmBgNVHREEHzAdhhtodHRwOi8vZXhhbXBsZS5vcmcvd2Vi\r\n' +
    'aWQjbWUwDAYIKoZIzj0EAwIFAAMVAFtvYmplY3QgQXJyYXlCdWZmZXJd\r\n' +
    '-----END CERTIFICATE-----\r\n',
  hash: <Buffer 3c ee 2c 2b e7 0d af df 92 92 0c fb 2a 5f 66 04 17 67 ba 0e c1 9a d1 26 1f 8f a1 4b c6 9e 34 e0>,
  fingerprint: '3C:EE:2C:2B:E7:0D:AF:DF:92:92:0C:FB:2A:5F:66:04:17:67:BA:0E:C1:9A:D1:26:1F:8F:A1:4B:C6:9E:34:E0'
}

Comment 1

[marten.richter]

Two other notes:
1.) The change does not check if the certificate is only valid for 14 days. (Security problem)
2.) It also does not implement, what Valentin Gosu had suggested:
"First of all we need the cert hashes to be passed from the content constructor all the way to the parent.
Then in AsyncConnectWithClient where we create the channel we need to pass the hashes into the channel and save them.
Then in nsHttpChannel::OnStartRequest, after mSecurityInfo is set we need to compute the actual hash of the certificate, then compare it with the serverCertificateHashes from the webtransport constructor and if they don't match to abort the channel."
It never does this.

I will look to see if I can fix this. I have already browsed through the code, and I have implemented this functionality at least 2 two somewhere else. It should not be so hard.

Comment 2

[marten.richter]

Attachment: Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation

r=kershaw

The current serverCertificateHashes implementation does not follow the
WebTransport specification, that introduced serverCertificateHashes
as a tool to replace certificate chain verification.
Instead it introduced the hashes as an additional check.
This patch moves the check to the Http3Session object and modifies
the connection manager' hashes to prevent crossSite certificate
poisoning. It is - as the WebTransport Implementation in Firefox -
currently limited to http3 only.
However, since the hashes live on the ConnectionInfo object,
it should be possible to extend this in the future.

Comment 3

[u738353]

The current serverCertificateHashes implementation does not follow the WebTransport specification, that introduced serverCertificateHashes as a tool to replace certificate chain verification.

Right, the merged patch implement it as an additional check, instead of replacing the certificate chain validation. Although I haven't read it explicitly in the spec, it makes sense and it's how the Chrome's implementation works.

Comment 4

[u738353]

(In reply to [e-mail] from comment #3)

The current serverCertificateHashes implementation does not follow the WebTransport specification, that introduced serverCertificateHashes as a tool to replace certificate chain verification.

Right, the merged patch implement it as an additional check, instead of replacing the certificate chain validation. Although I haven't read it explicitly in the spec, it makes sense and it's how the Chrome's implementation works.

This might be the spec prose related to this:

https://w3c.github.io/webtransport/#certificate-hashes

Since this mechanism substitutes Web PKI-based authentication for an individual connection, we need to compare the security properties of both.

Comment 5

[marten.richter]

Yes, correct it substitutes the "Web PKI-based authentication". So, the substitution is missing. Chromium also expects a self-signed certificate, and I can also point you (if you give me some time) to the class that substitutes the check in quiche (the underlying lib, I use it for my plugin).

Comment 6

[marten.richter]

I meant "accepts" instead of "expect".

Comment 7

[u738353]

2.) It also does not implement, what Valentin Gosu had suggested:

What part of that was not implemented ? As far as I know all the steps suggested there are part of the merged change.

I agree on the issue described in 1).

Comment 8

[u738353]

(In reply to [e-mail] from comment #7)

2.) It also does not implement, what Valentin Gosu had suggested:

What part of that was not implemented ? As far as I know all the steps suggested there are part of the merged change.

Did you meant the suggestion of storing the server hashes in the httpchannel ? I think that during the review process we decided to store them in the proxy instead; perhaps we can just pass the hashes to the nsConnectionInfo instance as well, and still keep the data stored in the proxy.

I'm worried about duplicating the data, though; I'd rather look for an alternative way of detecting that the http channel has been created with the hashes option and still avoid the Web PKI-based authentication" in that case.

Comment 9

[marten.richter]

(In reply to [e-mail] from comment #8)

(In reply to [e-mail] from comment #7)

2.) It also does not implement, what Valentin Gosu had suggested:

What part of that was not implemented ? As far as I know all the steps suggested there are part of the merged change.

Did you meant the suggestion of storing the server hashes in the httpchannel ? I think that during the review process we decided to store them in the proxy instead; perhaps we can just pass the hashes to the nsConnectionInfo instance as well, and still keep the data stored in the proxy.

I'm worried about duplicating the data, though; I'd rather look for an alternative way of detecting that the http channel has been created with the hashes option and still avoid the Web PKI-based authentication" in that case.

Yes, I meant this, but this was in the beginning when I tried to find out why my certificate was rejected.

The certificate verification is done within Http3Session, and I needed to bring the information down there so that no connection is established with the wrong certificates.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2dd957f853b
WebTransport: Fix serverCertificateHashes Implementation r=kershaw,necko-reviewers,keeler

Comment 12

[Sandor Molnar[:smolnar]]

Backed out for causing xpc failures @ netwerk/test/unit/test_webtransport_simple.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/ca5de2cea700f810aa89ebe1fee009d8110fa0c5

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_webtransport_simple.js | xpcshell return code: 0

Comment 13

[marten.richter]

Ok, I was not aware that the tests are calling AsyncConnect directly. In principle, I missed that the test (and other?) also need to be changed, adding simply a "true," after the first argument of all AsyncConnect should fix the test. However, it will take until the weekend it the holiday on Friday, until I have time to fix it.

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to marten.richter from comment #13)

Ok, I was not aware that the tests are calling AsyncConnect directly. In principle, I missed that the test (and other?) also need to be changed, adding simply a "true," after the first argument of all AsyncConnect should fix the test. However, it will take until the weekend it the holiday on Friday, until I have time to fix it.

Thanks for taking a look. It's not urgent, so please take your time.
If you prefer, I can also submit a patch to fix this.

Comment 15

[marten.richter]

(In reply to Kershaw Chang [:kershaw] from comment #14)

(In reply to marten.richter from comment #13)

Ok, I was not aware that the tests are calling AsyncConnect directly. In principle, I missed that the test (and other?) also need to be changed, adding simply a "true," after the first argument of all AsyncConnect should fix the test. However, it will take until the weekend it the holiday on Friday, until I have time to fix it.

Thanks for taking a look. It's not urgent, so please take your time.
If you prefer, I can also submit a patch to fix this.

I would appreciate, if you can patch it!
I am still very clumsy with hg and the other development tools and this save me a lot of time.

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Bug 1873263 - Disable 0RTT when servCertHashes is presented, r=#necko

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c508a42a91f6
WebTransport: Fix serverCertificateHashes Implementation r=kershaw,necko-reviewers,keeler
https://hg.mozilla.org/integration/autoland/rev/cc65c6e70547
Disable 0RTT when servCertHashes is presented, r=necko-reviewers,valentin

Comment 18

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c508a42a91f6
https://hg.mozilla.org/mozilla-central/rev/cc65c6e70547

Comment 19

[Manuel Bucher [:manuel]]

options.serverCertificateHashes parameter is currently marked as unsupported on mdn: https://developer.mozilla.org/en-US/docs/Web/API/WebTransport. This probably needs to be updated.