Closed Bug 1456489 Opened 2 years ago Closed 2 years ago

prohibit making OCSP requests on the main thread

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

The current OCSP implementation performs nested event loop spinning if an OCSP request is made on the main thread, which can cause problems. The only other option to handle main-thread OCSP requests would be to block the main thread, and since that's not a reasonable solution, this behavior must be prevented. Luckily, as of bug 867473, all verification happens off the main thread, so we can just forbid this.
Comment on attachment 8970812 [details]
bug 1456489 - prevent making OCSP requests on the main thread

https://reviewboard.mozilla.org/r/239310/#review245256


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: security/manager/ssl/nsNSSCallbacks.cpp:135
(Diff revision 1)
> +  , mResponseResult(NS_ERROR_FAILURE)
> +  , mResponseBytes()
>  {
>  }
>  
> -nsHTTPDownloadEvent::~nsHTTPDownloadEvent()
> +OCSPRequest::~OCSPRequest()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

 0:10.57 OCSPRequest::~OCSPRequest()
 0:10.57              ^
 0:10.57 Warning: modernize-use-auto in /cache/sa-central/security/manager/ssl/nsNSSCallbacks.cpp: use auto when initializing with a cast to avoid duplicating the type name

::: security/manager/ssl/nsNSSCallbacks.cpp:135
(Diff revision 1)
> +  , mResponseResult(NS_ERROR_FAILURE)
> +  , mResponseBytes()
>  {
>  }
>  
> -nsHTTPDownloadEvent::~nsHTTPDownloadEvent()
> +OCSPRequest::~OCSPRequest()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

 0:10.61 OCSPRequest::~OCSPRequest()
 0:10.61              ^
 0:10.62 Warning: modernize-use-auto in /cache/sa-central/security/manager/ssl/nsNSSCallbacks.cpp: use auto when initializing with a cast to avoid duplicating the type name
Comment on attachment 8970812 [details]
bug 1456489 - prevent making OCSP requests on the main thread

https://reviewboard.mozilla.org/r/239310/#review245260

Sorry for the delay.
I feel like I should've found issues in here but I didn't :)

::: security/certverifier/NSSCertDBTrustDomain.cpp:340
(Diff revision 1)
>            // but we limit it to a smaller bound to reduce OOM risk.
>            if (location.len > 1024 || memchr(location.data, 0, location.len)) {
>              // Reject embedded nulls. (NSS doesn't do this)
>              return Result::ERROR_CERT_BAD_ACCESS_LOCATION;
>            }
> -          // Copy the non-null-terminated SECItem into a null-terminated string.
> +          result.Assign(nsDependentCSubstring(

This adds the null byte at the end?

::: security/manager/ssl/nsNSSCallbacks.cpp:121
(Diff revision 2)
> -  : mozilla::Runnable("nsHTTPDownloadEvent")
> -  , mResponsibleForDoneSignal(true)
> +
> +OCSPRequest::OCSPRequest(const nsCString& aiaLocation,
> +                         const OriginAttributes& originAttributes,
> +                         Vector<uint8_t>&& ocspRequest,
> +                         TimeDuration timeout)
> +  : mMonitor("OCSPRequest.mMonitor")

I think using C++11 initialisation in the header is becoming more popular. It safes space and looks nicer imho. Not a big deal though.

::: security/manager/ssl/nsNSSCallbacks.cpp:298
(Diff revision 2)
> -
> -    rv = uploadChannel->SetUploadStream(uploadStream,
> -                                        mRequestSession->mPostContentType,
> -                                        -1);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(channel));
> +  if (!uploadChannel) {
> +    return NotifyDone(NS_ERROR_FAILURE, lock);
> +  }
> +  rv = uploadChannel->SetUploadStream(
> +         uploadStream, NS_LITERAL_CSTRING("application/ocsp-request"), -1);

Maybe make this and other strings here constants?

::: security/manager/ssl/nsNSSCallbacks.cpp:397
(Diff revision 2)
> -             "thread; this will not work.");
> +    return NotifyDone(NS_ERROR_FAILURE, lock);
> -    return Result::FATAL_ERROR_INVALID_STATE;
>    }
>  
> -  const int max_retries = 2;
> -  int retry_count = 0;
> +  bool requestSucceeded;
> +  rv = hchan->GetRequestSucceeded(&requestSucceeded);

You're not actually checking `requestSucceeded`?

::: security/manager/ssl/nsNSSCallbacks.cpp:421
(Diff revision 2)
>  
> -  return rv;
> +  return NotifyDone(NS_OK, lock);
>  }
>  
>  void
> -nsNSSHttpRequestSession::AddRef()
> +OCSPRequest::OnTimeout(nsITimer* timer, void* closure)

I guess it's guaranteed that `OnStreamComplete` won't be called after timeout?
Attachment #8970812 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8970812 [details]
bug 1456489 - prevent making OCSP requests on the main thread

https://reviewboard.mozilla.org/r/239310/#review246520

I don't see anything to block this, it looks pretty good. I specifically verified that the member variables protected by your monitor were always protected: looks good. I think it's correct.

Unfortunately I think I did you a disservice last week, because I think you could have saved yourself a bunch of pain had you done this with `MozPromise`, as it is a straightforward task for those. But I wouldn't recommend converting this, it looks fine as-is. Just remember for next time we need to go async.

Good work!

::: security/certverifier/NSSCertDBTrustDomain.cpp:304
(Diff revision 2)
>    MOZ_ASSERT_UNREACHABLE("we're not handling every OCSPFetching type");
>    return mOCSPTimeoutSoft;
>  }
>  
>  // Copied and modified from CERT_GetOCSPAuthorityInfoAccessLocation and
>  // CERT_GetGeneralNameByType. Returns a non-Result::Success result on error,

Please also note that `result` is dependent on `aiaExtension` in the documentation.

::: security/manager/ssl/nsNSSCallbacks.cpp:229
(Diff revision 2)
> +  ToLowerCase(scheme);
> +  if (!scheme.EqualsLiteral("http")) {

For future reference, remember `LowerCaseEqualsLiteral`

::: security/manager/ssl/nsNSSCallbacks.cpp:429
(Diff revision 2)
> +  if (!NS_IsMainThread()) {
> +    return;
> -}
> +  }
>  
> -void
> -nsNSSHttpRequestSession::Release()
> +  OCSPRequest* self = static_cast<OCSPRequest*>(closure);
> +  MOZ_ASSERT(timer == self->mTimeoutTimer);

This is kind of a weird thing to assert. If we're concerned about `closure` being mixed up with adifferent `timer`, we should probably check that outside of the `MOZ_ASSERT` too.
Attachment #8970812 - Flags: review?(jjones) → review+
Comment on attachment 8970812 [details]
bug 1456489 - prevent making OCSP requests on the main thread

https://reviewboard.mozilla.org/r/239310/#review245260

No worries - thanks for the review!

> This adds the null byte at the end?

My understanding is it ends up copying the data, since we want `result` to own it. In any case, yes, it should be null-terminated.

> I think using C++11 initialisation in the header is becoming more popular. It safes space and looks nicer imho. Not a big deal though.

I had a look at the email thread on dev.platform and I agree with bz - using {} initializers in constructors can make it difficult to see where the actual constructor begins, so I think I'll keep this as-is for now.

> Maybe make this and other strings here constants?

Sounds good.

> You're not actually checking `requestSucceeded`?

Whoops - good catch.

> I guess it's guaranteed that `OnStreamComplete` won't be called after timeout?

It can be called after timeout, I think, but it would be a no-op because `NotifyDone` short-circuits if it's run more than once.
Comment on attachment 8970812 [details]
bug 1456489 - prevent making OCSP requests on the main thread

https://reviewboard.mozilla.org/r/239310/#review246520

Thanks!

> Please also note that `result` is dependent on `aiaExtension` in the documentation.

It's not, unless I'm misunderstanding `Assign`? It should copy the data (since it can't move or share it).

> For future reference, remember `LowerCaseEqualsLiteral`

Ah - good call.

> This is kind of a weird thing to assert. If we're concerned about `closure` being mixed up with adifferent `timer`, we should probably check that outside of the `MOZ_ASSERT` too.

Eh, I was just cargo-culting this. I can remove it.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47d306cfac90
prevent making OCSP requests on the main thread r=fkiefer,jcj
https://hg.mozilla.org/mozilla-central/rev/47d306cfac90
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1460312
Depends on: 1497258
See Also: → 1576364
You need to log in before you can comment on or make changes to this bug.