Open Bug 1512471 Opened 1 year ago Updated 4 days ago

Proxy certificate validation from Socket Process/Socket Thread to Parent Process

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

ASSIGNED
Tracking Status
firefox65 --- affected

People

(Reporter: mayhemer, Assigned: kershaw)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files, 8 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
No description provided.
Attached patch wip, backup, needs rebasing (obsolete) — Splinter Review
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [psm-assigned]
  • please use this patch only for getting some base parts necessary for the IPC plumbing between the processes to carry on the cert verification
  • there is PSMChild and *Parent added to manage the new PAuthCertificate protocol providing async messages for cert verification
  • there are also some base tools for serialization added
  • unfinished: we need to carry some more properties like the host name we verify against via the new PAuthCertificate protocol (host name - a property of the info object - is not serialized)
  • definitely unfinished: this needs PBackground or end-points to send from the socket thread on the socket process to some thread on the parent process and back

I never made this work even slightly, so there is probably a lot of work to do...

Attachment #9029844 - Attachment is obsolete: true
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Attachment #9039872 - Attachment is obsolete: true
Attachment #9058429 - Attachment is obsolete: true

keeler, can you now review this patches?

Flags: needinfo?(dkeeler)

I can start to look at these again, yes. Are you going to land the patches from bug 1546816 on mozilla-central?

Flags: needinfo?(dkeeler)

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #8)

I can start to look at these again, yes. Are you going to land the patches from bug 1546816 on mozilla-central?

I though of landing it on larch, but mozilla-central is fine too, probably better.

Assignee: dd.mozilla → kershaw

Hi Dana,

When I tested the patches in this bug, I found something strange. When I tried to connect to www.mysecurex.eu, I always got SEC_ERROR_UNKNOWN_ISSUER. This error returned from PathBuildingStep::CheckResult and the call stack is:

frame #0: 0x00000001153030ce XUL`mozilla::pkix::BuildForward(mozilla::pkix::TrustDomain&, mozilla::pkix::BackCert const&, mozilla::pkix::Time, mozilla::pkix::KeyUsage, mozilla::pkix::KeyPurposeId, mozilla::pkix::CertPolicyId const&, mozilla::pkix::Input const*, unsigned int, unsigned int&) [inlined] mozilla::pkix::PathBuildingStep::CheckResult() const at pkixbuild.cpp:135:8 [opt]
frame #1: 0x00000001153030ce XUL`mozilla::pkix::BuildForward(trustDomain=0x00007000113954b0, subject=0x00007000113951f8, time=(elapsedSecondsAD = 63703127818), requiredKeyUsageIfPresent=<unavailable>, requiredEKUIfPresent=id_kp_serverAuth, requiredPolicy=0x000000011877aa5e, stapledOCSPResponse=0x0000700011395550, subCACount=<unavailable>, buildForwardCallBudget=0x00007000113951f4) at pkixbuild.cpp:375 [opt]
frame #2: 0x00000001153032a5 XUL`mozilla::pkix::BuildCertChain(trustDomain=0x00007000113954b0, certDER=<unavailable>, time=(elapsedSecondsAD = 63703127818), endEntityOrCA=MustBeEndEntity, requiredKeyUsageIfPresent=digitalSignature, requiredEKUIfPresent=id_kp_serverAuth, requiredPolicy=0x000000011877aa5e, stapledOCSPResponse=0x0000700011395550) at pkixbuild.cpp:422:10 [opt]
frame #3: 0x000000011002424e XUL`mozilla::psm::BuildCertChainForOneKeyUsage(trustDomain=0x00007000113954b0, certDER=<unavailable>, time=(elapsedSecondsAD = 63703127818), ku1=digitalSignature, ku2=keyEncipherment, ku3=keyAgreement, eku=id_kp_serverAuth, requiredPolicy=0x000000011877aa5e, stapledOCSPResponse=0x0000700011395550, ocspStaplingStatus=0x0000700011395738) at CertVerifier.cpp:209:7 [opt]
frame #4: 0x00000001100237f0 XUL`mozilla::psm::CertVerifier::VerifyCert(this=0x0000000132148000, cert=<unavailable>, usage=<unavailable>, time=(elapsedSecondsAD = 63703127818), pinArg=0x0000000000000000, hostname="aus5.mozilla.org", builtChain=0x0000700011395750, flags=0, stapledOCSPResponseArg=0x0000000132591d10, sctsFromTLS=0x0000000132591d20, originAttributes=0x0000000132591d58, evOidPolicy=0x000070001139574c, ocspStaplingStatus=0x0000700011395738, keySizeStatus=0x0000700011395748, sha1ModeResult=0x0000700011395744, pinningTelemetryInfo=0x0000700011395758, ctInfo=0x0000700011395770) at CertVerifier.cpp:711:16 [opt]
frame #5: 0x000000011002447a XUL`mozilla::psm::CertVerifier::VerifySSLServerCert(this=0x0000000132148000, peerCert=0x0000000132591d08, stapledOCSPResponse=0x0000000132591d10, sctsFromTLS=<unavailable>, time=<unavailable>, pinarg=0x0000000000000000, hostname=0x0000000132591d38, builtChain=0x0000700011395750, saveIntermediatesInPermanentDatabase=<unavailable>, flags=0, originAttributes=0x0000000132591d58, evOidPolicy=0x000070001139574c, ocspStaplingStatus=0x0000700011395738, keySizeStatus=0x0000700011395748, sha1ModeResult=0x0000700011395744, pinningTelemetryInfo=0x0000700011395758, ctInfo=0x0000700011395770) at CertVerifier.cpp:893:7 [opt]
frame #6: 0x00000001152ad8ee XUL`mozilla::psm::VerifySSLServerCertParent::Run(this=<unavailable>) at VerifySSLServerCertParent.cpp:96:25 [opt]

In NSSCertDBTrustDomain::FindIssuer, I also found CERT_CreateSubjectCertList returns an empty cert list. But CERT_CreateSubjectCertList returns a cert list when socket process is disabled.
So, I compared the input encodedIssuerNameItem to CERT_CreateSubjectCertList with and without socket process, and it's the same.
Do you know what else could make CERT_CreateSubjectCertList return null? Is there anything I missed?

Thanks!

Flags: needinfo?(dkeeler)

I'm assuming this is because the parent process can't find the intermediate certificates that the server sent in the TLS handshake, because only the socket process know about them. This is why it's necessary to call SSL_PeerCertificateChain(fd) in the socket process and send that data to the parent: https://phabricator.services.mozilla.com/D28743#1335802

Flags: needinfo?(dkeeler)

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #11)

I'm assuming this is because the parent process can't find the intermediate certificates that the server sent in the TLS handshake, because only the socket process know about them. This is why it's necessary to call SSL_PeerCertificateChain(fd) in the socket process and send that data to the parent: https://phabricator.services.mozilla.com/D28743#1335802

Yes, you are right! After sending the peer cert chain to parent process and calling CERT_NewTempCertificate for each cert, it works.
Thanks for your help. I'll update the patch.

Attachment #9060558 - Attachment is obsolete: true
Attachment #9104698 - Attachment description: Bug 1512471 - Add code for deserializing and serializing CertificateTransparencyInfo → Bug 1512471 - Add a helper function to convert CertificateTransparencyInfo to CertificateTransparencyStatus
Depends on: 1607194
Attachment #9060560 - Attachment description: Part 1: Perform cert verifications on the parent process. r=keeler → Perform cert verifications on the parent process. r=keeler
Blocks: 1602832
Attachment #9118508 - Attachment description: Bug 1512471 - Using CertBytes array to replace UniqueCERTCertList → Bug 1512471 - Using arrays of array bytes to replace UniqueCERTCertList
Attachment #9060560 - Attachment is obsolete: true

Comment on attachment 9104698 [details]
Bug 1512471 - Add a helper function to convert CertificateTransparencyInfo to CertificateTransparencyStatus

Revision D50833 was moved to bug 1612362. Setting attachment 9104698 [details] to obsolete.

Attachment #9104698 - Attachment is obsolete: true

Comment on attachment 9118508 [details]
Bug 1512471 - Using arrays of array bytes to replace UniqueCERTCertList

Revision D58608 was moved to bug 1612362. Setting attachment 9118508 [details] to obsolete.

Attachment #9118508 - Attachment is obsolete: true

Comment on attachment 9118501 [details]
Bug 1512471 - Refactor SSLServerCertVerificationJob for reusing the code

Revision D58604 was moved to bug 1612362. Setting attachment 9118501 [details] to obsolete.

Attachment #9118501 - Attachment is obsolete: true
Depends on: 1603420
You need to log in before you can comment on or make changes to this bug.