Closed Bug 1003566 Opened 10 years ago Closed 10 years ago

OCSP requests should never be upgraded to https by HSTS

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: keeler, Assigned: keeler)

Details

Attachments

(2 files, 4 obsolete files)

We've had a report of encountering sec_error_ocsp_unknown_cert on mail.google.com using Nightly 32. I haven't been able to reproduce this. It may be an intermittent issue, or it may be network-topology-specific, or something else.
mcmanus also has a zip of my NSPR logs.
bent has had some other networking issues and he's win 8.1 based.. just tossing it out there in case it relevant.
At first we thought it was a proxy issue, right? But I have turned the setting to "No proxy" in ff and this issue persists.
Is it consistent or intermittent?
I usually have to restart FF a few times to get it to happen. On reload I have about four pinned tabs that load (two google, one zimbra, another thing that probably uses some google apis) and a bugzilla seems to be focused frequently.
:bent - your latest log is defintely not a proxy issue. Those callbacks happen immediately.

OCSP seems to be happening with HSTS! I sense a bootstrap problem.

22.746: Creating HttpBaseChannel @21ecb000
22.747: Creating nsHttpChannel [this=21ecb000]
22.747: HttpBaseChannel::Init [this=21ecb000]
22.747: host=clients1.google.com port=-1
22.747: uri=http://clients1.google.com/ocsp
22.747: nsHttpChannel::Init [this=21ecb000]
22.747: HttpBaseChannel::SetRequestHeader [this=21ecb000 header="Content-Length" value="75" merge=0]
22.747: HttpBaseChannel::SetRequestHeader [this=21ecb000 header="Content-Type" value="application/ocsp-request" merge=0]
22.747: nsHttpChannel::AsyncOpen [this=21ecb000]
22.747: nsHttpHandler::NotifyObservers [chan=21ecb030 event="http-on-opening-request"]
22.747: nsHttpChannel::ResolveProxy [this=21ecb000]
22.747: nsHttpChannel::OnProxyAvailable [this=21ecb000 pi=0 status=0 mStatus=0]
22.747: nsHttpChannel::BeginConnect [this=21ecb000]
22.747: host=clients1.google.com port=-1
22.747: uri=http://clients1.google.com/ocsp
22.747: Creating nsHttpConnectionInfo @6a2df40
22.747: nsHttpChannel::BeginConnect [this=21ecb000] prefetching
22.747: Resolving host [clients1.google.com].
22.747:   Using cached record for host [clients1.google.com].
22.747: nsHttpChannel::Connect [this=21ecb000]
22.747: nsHttpChannel::Connect() STS permissions found
22.748: nsHttpChannel::OnLookupComplete [this=21ecb000] prefetch complete: success status[0x0]
22.748: nsHttpChannel::HandleAsyncRedirectChannelToHttps() [STS]
22.748: nsHttpChannel::StartRedirectChannelToURI()
22.748: nsHttpHandler::NewProxiedChannel [proxyInfo=0]
22.749: Creating HttpBaseChannel @d38c000
[...]
in :bent's log there are 230 transactions to https://mail.google.com and also 229 OCSP queries.. they seem to be gumming up the works and breaking other https uses (like bugzilla) for him, A couple dozen ocsp transactions actually get submitted to necko (and canceled) after shutdown starts - as if a queue were being aborted.
A couple of thoughts:

1. It looks like we don't prevent http -> https upgrading due to HSTS for OCSP requests. This is definitely something we need to fix. That said, clients1.google.com doesn't appear to be sending an HSTS header and it isn't on the preload list, so I'm not sure how that would be what's causing this.
2. As far as I know, we don't prevent concurrent OCSP requests of the same certificate, which is a huge waste of time/network resources. It would be great if we could fix this too.
I guess you can check that you really do have sts permissions for the ocsp server in permissions.sqlite..

assuming you do, we need to wonder how that happened.. I have no idea how to investigate that given its persistnece. david/camillo/sid might

PSM could certainly block the internal redirect to https but that wouldn't make the original http channel go through. If it makes sense to punch a hole for ocsp through sts we can make an interface for that.. (wtf - we have a hardcoded hole for chart.apis.google.com?)
Aha! I used sid's addon (http://forcetls.sidstamm.com) to add *.google.com and *.facebook.com a long time ago.
Updating summary and bug dependency based on my understanding of the issue.
No longer blocks: mozilla::pkix-beta
Summary: mozilla::pkix: sec_error_ocsp_unknown_cert for mail.google.com → OCSP requests should never be upgraded to https by HSTS
david, hopefully this channel flag will let PSM do the necessary thing. let me know if if works for you and we can review it and put it in the tree.
Comment on attachment 8422493 [details] [diff] [review]
0001-bug-1003566-part-1-allowSTS-attribute-to-nsIHttpChan.patch

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

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +133,5 @@
> +     * This attribute may only be set before the channel is opened.
> +     *
> +     * @throws NS_ERROR_FAILURE if set after the channel has been opened.
> +     */
> +    attribute boolean allowSTS;

note to self - requires an idl uuid bump
forgot to add the passthrough for the view source channel
Attachment #8422493 - Attachment is obsolete: true
I think that's exactly what we need, yes. Thanks!
Hey guys, what's the status here? I'd like to reenable the new library so I can do more nightly testing :)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #10)
> Aha! I used sid's addon (http://forcetls.sidstamm.com) to add *.google.com
> and *.facebook.com a long time ago.

This is not a supported configuration. :)
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #17)
> This is not a supported configuration. :)

TBH I was convinced some of their sites would break, but so far everything seems to work ok!
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #18)
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> from comment #17)
> > This is not a supported configuration. :)
> 
> TBH I was convinced some of their sites would break, but so far everything
> seems to work ok!

I understand. We should fix this bug. But, I don't think that this needs to be a high priority, because it isn't negatively affecting anybody in a normal ("supported") configuration.
This applies on top of the other patch in this bug.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8432862 - Flags: review?(cviecco)
Comment on attachment 8432862 [details] [diff] [review]
part 2 - nsNSSCallbacks part + test

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

::: security/manager/ssl/tests/unit/test_ocsp_no_hsts_upgrade.js
@@ +50,5 @@
> +                               useMozillaPKIX);
> +    run_next_test();
> +  });
> +
> +  add_connection_test("ocsp-stapling-none.example.com", Cr.NS_OK);

I would add a note here explaining where the certs are generated (seems a little bit strange that we dont know here the information about the cert).
Attachment #8432862 - Flags: review?(cviecco) → review+
Attachment #8422620 - Flags: review?(honzab.moz)
Comment on attachment 8422620 [details] [diff] [review]
0001-bug-1003566-part-1-allowSTS-attribute-to-nsIHttpChan.patch

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

Patrick, please next time submit patches with 8 line context as everybody else does.  This patch is simple, but for anything more complex it makes reviews much harder.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +131,5 @@
> +     * false.
> +     *
> +     * This attribute may only be set before the channel is opened.
> +     *
> +     * @throws NS_ERROR_FAILURE if set after the channel has been opened.

NS_ERROR_ALREADY_OPENED or NS_ERROR_IN_PROGRESS

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +651,5 @@
>  NS_IMETHODIMP
> +nsViewSourceChannel::GetAllowSTS(bool *aAllowSTS)
> +{
> +    return !mHttpChannel ? NS_ERROR_NULL_POINTER :
> +        mHttpChannel->GetAllowSTS(aAllowSTS);

wish the file style was:

return mHttpChannel
  ? mHttpChannel->GetAllowSTS(aAllowSTS)
  : NS_ERROR_NULL_POINTER;
Attachment #8422620 - Flags: review?(honzab.moz) → review+
Attached patch part 1 (obsolete) — Splinter Review
ready for checkin when david wants..
Attachment #8422620 - Attachment is obsolete: true
Attachment #8433625 - Flags: review+
Attached patch part 1Splinter Review
There was a small typo in part 1 (a block comment in the idl file wasn't closed).
Attachment #8433625 - Attachment is obsolete: true
Attachment #8434272 - Flags: review+
Attached patch part 2Splinter Review
Added comments. Carrying over r+.
Attachment #8432862 - Attachment is obsolete: true
Attachment #8434273 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d5c4fb8b8995
https://hg.mozilla.org/mozilla-central/rev/5c901d7e88bc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.