Closed Bug 1054347 Opened 10 years ago Closed 8 years ago

don't accept h2 in npn list if < TLS 1.2 is being used

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Version:
32 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Whiteboard: [sdpy])

Attachments

(2 files)

part 1 psm use npn callback function instead of deferring to nss
15.79 KB, patch
keeler
: review-
Details | Diff | Splinter Review
part 2 necko use npn callback function instead of deferring to nss
9.88 KB, patch
u408661
: review+
Details | Diff | Splinter Review 
This is a followup to 1050063

we accept h2 from the NPN offer list even though we received a 1.0 Hello in response to a 1.2 client hello.
Depends on: 1054349
hmmm.. stalled on a nss link problem on windows

nsNSSIOLayer.obj : error LNK2019: unresolved external symbol __imp__SSL_SetNextProtoCallback referenced in function "public: virtual enum tag_nsresult __stdcall nsNSSSocketInfo::SetNPNList(class nsTArray<class nsCString> &,class nsINPNValidator *)" (?SetNPNList@nsNSSSocketInfo@@UAG?AW4tag_nsresult@@AAV?$nsTArray@VnsCString@@@@PAVnsINPNValidator@@@Z)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
and a try run that figured out the ref counted cycle - https://tbpl.mozilla.org/?tree=Try&rev=ae3114a24442
Comment on attachment 8475592 [details] [diff] [review]
part 2 necko use npn callback function instead of deferring to nss

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

r=me with the removal/change noted below

::: netwerk/protocol/http/ASpdySession.cpp
@@ +29,5 @@
>  namespace net {
>  
> +//////////////////////////////////////////
> +/// ASdpySession
> +//////////////////////////////////////////

This now looks like the fastest comment in the world :)

::: netwerk/protocol/http/Http2Session.cpp
@@ +2948,5 @@
>  }
>  
> +
> +bool // static
> +Http2Session::NPNCallback(nsISupports *securityInfo)

Let's merge this with ALPNCallback, since they're literally the exact same check. Sure we lose the logging distinction between NPN/ALPN, but given that the NPN check will have another log statement referencing NPN right after it, I'm willing to make that tradeoff.
Attachment #8475592 - Flags: review?(hurley) → review+
(In reply to Nicholas Hurley [:hurley] from comment #9)
> Comment on attachment 8475592 [details] [diff] [review]
> part 2 necko use npn callback function instead of deferring to nss
> 
> Review of attachment 8475592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the removal/change noted below
> 
>
> Let's merge this with ALPNCallback, since they're literally the exact same
> check. 

they're not quite the same.. one checks version offered and the other checks version used.
(In reply to Patrick McManus [:mcmanus] from comment #10)
> they're not quite the same.. one checks version offered and the other checks
> version used.

*facepalm* yep. please ignore my temporary idiocy.
Comment on attachment 8475591 [details] [diff] [review]
part 1 psm use npn callback function instead of deferring to nss

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

This would be an r+, but I think we should treat the data in the NSS callback with a bit more suspicion. In particular, we're trusting that data from the peer has already been validated correctly, right? I think we should probably be a bit more paranoid here. There are some utilities in mozilla::pkix that should provide an appropriate interface for reading these values in a fast, memory-safe way. I think you can do something like this:

Input inputProtos(protos, protosLen);
Input inputAllowed(allowedString, allowedStringLength);
Reader readerProtos(inputProtos);
Reader readerAllowed(inputAllowed);
while (!found && !readerProtos.AtEnd() && !readerAllowed.AtEnd()) {
  uint8_t protosItemLen;
  readerProtos.Read(protosItemLen);
  uint8_t allowedItemLen;
  readerAllowed.Read(allowedItemLen);
  if (protosItemLen == allowedItemLen) {
    Input skippedProtos;
    readerProtos.Skip(protosItemLen, skippedProtos);
    Input skippedAllowed;
    readerAllowed.Skip(readerItemLen, skippedAllowed);
    if (InputsAreEqual(skippedProtos, skippedAllowed)) {
      found = true;
    }
  }
}

(I think you'll have to do some more error-checking on each operation, but you get the idea. See security/pkix/include/pkix/Input.h)

::: netwerk/socket/nsISSLSocketControl.idl
@@ +29,5 @@
> +     * negotiating the protocol to be spoken inside the SSL
> +     * tunnel during the SSL handshake. The NPNList is the list
> +     * of offered client side protocols. setNPNList() needs to
> +     * be called before any data is read or written (including the
> +     * handshake to be setup correctly. Selection is made by the

nit: "handshake)", I think?

@@ +116,5 @@
>       */
>      attribute nsIX509Cert clientCert;
>  };
>  
> +

nit: we probably don't need this extra blank line

@@ +123,5 @@
> +{
> +  boolean ValidateNPNChoice(in nsACStringRef npnProtocol,
> +                            in nsISSLSocketControl socketControl);
> +};
> +

nit: trailing blank line

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +915,5 @@
> +          state == SSL_NEXT_PROTO_SELECTED) {
> +        infoObject->SetNegotiatedNPN(reinterpret_cast<char *>(alpnbuf),
> +                                     alpnlen, true);
> +      }
> +      else {

nit: } else {

@@ +923,2 @@
>      }
>      else {

nit: It would be nice if this were all on line line as well ("} else {")

@@ +1065,5 @@
>  }
>  
> +SECStatus
> +NPNCallback(void* closure, PRFileDesc* fd,
> +            const unsigned char* protos, unsigned int protos_len,

nit: protosLen to be consistent

@@ +1069,5 @@
> +            const unsigned char* protos, unsigned int protos_len,
> +            unsigned char* protoOut, unsigned int* protoOutLen,
> +            unsigned int protoMaxOut)
> +{
> +  nsNSSSocketInfo* infoObject = static_cast<nsNSSSocketInfo *>(closure);

nit: static_cast<nsNSSSocketInfo*>

@@ +1071,5 @@
> +            unsigned int protoMaxOut)
> +{
> +  nsNSSSocketInfo* infoObject = static_cast<nsNSSSocketInfo *>(closure);
> +  nsNSSShutDownPreventionLock locker;
> +  MOZ_ASSERT(infoObject == (nsNSSSocketInfo*) fd->higher->secret);

Hmmm - if fd->higher->secret is supposed to be the same infoObject, why bother with the closure?

@@ +1077,5 @@
> +    PR_SetError(PR_INVALID_STATE_ERROR, 0);
> +    return SECFailure;
> +  }
> +
> +  // modeled very closely on NSS sslsock::ssl_NextProtoNegoCallback

Honestly, I don't think that's the best way to approach this. I wonder if some of the utilities from mozilla::pkix (like Input/Reader) would be helpful here. In particular, I'm concerned about memory safety and reading out of bounds.

@@ +1078,5 @@
> +    return SECFailure;
> +  }
> +
> +  // modeled very closely on NSS sslsock::ssl_NextProtoNegoCallback
> +  const nsCString *allowedString = infoObject->GetNPNAllowedStringPtr();

If allowedString can't be null, why not just use a reference? (alternatively, if it can be null, we should error-check that)

@@ +1079,5 @@
> +  }
> +
> +  // modeled very closely on NSS sslsock::ssl_NextProtoNegoCallback
> +  const nsCString *allowedString = infoObject->GetNPNAllowedStringPtr();
> +  nsINPNValidator *validator = infoObject->GetNPNValidator();

nit: * to the left on these two lines

@@ +1093,5 @@
> +  /* For each protocol in server preference, see if we support it. */
> +  for (uint32_t i = 0; i < protos_len; ) {
> +    for (uint32_t j = 0; j < allowedString->Length(); ) {
> +      if (protos[i] == (allowedString->get())[j] &&
> +          !memcmp(&protos[i+1], allowedString->get() + j + 1, protos[i])) {

I know ssl.h says the input data will be well-formed and spec-compliant, but we really shouldn't trust it. In particular, I think we could read beyond both buffers here.

@@ +1104,5 @@
> +        if (npnValidated) {
> +          /* We found a match and it is still ok with the rest of gecko */
> +          match = true;
> +          result = reinterpret_cast<const char*>(&protos[i]);
> +          goto found;

I think we could get rid of this goto pretty easily by using a bool found and adding && !found to the loops.

::: security/manager/ssl/src/nsNSSCallbacks.h
@@ +26,5 @@
>  void HandshakeCallback(PRFileDesc *fd, void *client_data);
>  SECStatus CanFalseStartCallback(PRFileDesc* fd, void* client_data,
>                                  PRBool *canFalseStart);
> +SECStatus NPNCallback(void* closure, PRFileDesc* fd,
> +                      const unsigned char* protos, unsigned int protos_len,

nit: protosLen to be consistent

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +482,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsNSSSocketInfo::SetNPNList(nsTArray<nsCString>& protocolArray,
> +                            nsINPNValidator *validator)

nit: nsINPNValidator*

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +113,5 @@
>  
>    void SetMACAlgorithmUsed(int16_t mac) { mMACAlgorithmUsed = mac; }
>  
> +  const nsCString *GetNPNAllowedStringPtr() const { return &mNPNString; }
> +  nsINPNValidator *GetNPNValidator() const { return mNPNValidator.get(); }

nit: move the * one space left on these two lines

@@ +134,5 @@
>    bool mPreliminaryHandshakeDone; // after false start items are complete
>  
>    nsresult ActivateSSL();
>  
> +  nsCString mNPNString;      // a NPN formatted offer string. See ssl.h

nit: we probably only need one space after the ";" before the comment. Not a big deal, though.
Attachment #8475591 - Flags: review?(dkeeler) → review-
(In reply to David Keeler (:keeler) [use needinfo?] from comment #12)
> 
> Honestly, I don't think that's the best way to approach this. I wonder if
> some of the utilities from mozilla::pkix (like Input/Reader) would be
> helpful here. In particular, I'm concerned about memory safety and reading
> out of bounds.
> 

thanks David - yes, the callback is quite ugly. Its only redeeming feature is that can be compared to the internal NSS version and the minor change in semantics it makes is pretty clear, which is why I did it like that. It sounds like you would rather have it just stand on it own in a more modern style - I'm fine with that and will update.
more likely to remove npn first than make the changes necessary to do this
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: