Closed Bug 1873263 Opened 5 months ago Closed 3 months ago

Webtransport: serverCertificateHashes does not work as expected

Categories

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

defect

Tracking

()

RESOLVED FIXED
125 Branch
Tracking Status
firefox125 --- fixed

People

(Reporter: marten.richter, Assigned: marten.richter)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

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'
}

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.

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.

Assignee: nobody → marten.richter
Attachment #9371449 - Attachment description: WIP: Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation → Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9371449 - Attachment description: Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation → Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation r?kershaw
Attachment #9371449 - Attachment description: Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation r?kershaw → Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation

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.

(In reply to jfernandez@igalia.com 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.

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

I meant "accepts" instead of "expect".

Severity: -- → S3
Priority: -- → P2
Whiteboard: [necko-triaged]

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

(In reply to jfernandez@igalia.com 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.

(In reply to jfernandez@igalia.com from comment #8)

(In reply to jfernandez@igalia.com 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.

Blocks: 1790674
No longer blocks: 1806693
Depends on: 1806693
Attachment #9371449 - Attachment description: Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation → Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation r?kershaw
Attachment #9371449 - Attachment description: Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation r?kershaw → Bug 1873263 - WebTransport: Fix serverCertificateHashes Implementation
Duplicate of this bug: 1882331
Pushed by kjang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2dd957f853b
WebTransport: Fix serverCertificateHashes Implementation r=kershaw,necko-reviewers,keeler

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.

Flags: needinfo?(marten.richter)

(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.

(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.

Pushed by kjang@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch

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.

Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: