Security review notes for followup

[randomdross]

I did a quick security-focused review and these are my notes. PTAL and assess if there's anything that makes sense to follow up on. (This is similar to my review of PMJSON).

---

• I’m not Swift savvy so I may have missed something, but the following documents what I looked into / pondered and some outstanding questions.
• Does it follow server-side redirects? Is this configurable?
  • Looks like it, here:
• CR/LF injection in headers? (Or in URL paths too for that matter)
  • Could this be an issue in HTTPAuth.swift? I see data here that seems to land in headers.
  • Seems to be handled appropriately by the quotedString function in HTTPBodyStream.swift at least.
• What happens if cert validation fails? (Does it fail secure by default?)
• Most web security considerations seem not relevant in this context. (E.g.: CORS) Though I wonder if it makes sense to explicitly support HSTS or anything else, in any way?
• Some cookie-related code in HTTPManagerRequest.swift
  • Not sure what’s happening here:
    • ...what happens if the oldValue cookie string actually has a semicolon in it?
  • Some support for policy related to third party cookies
• Re caching -- it is conceivable to mess this up, you can imagine if a cached HTTP resource could somehow be retrieved on a subsequent HTTPS request to the same origin / path.
• Search for “unsafe”
  • 17 results
  • Some of these are in test code, don’t really care about those.
  • Didn’t pin down specific memory corruption issues, but that’s the sort of thing I’d be worried about for any of these.
• SipHash implementation reports itself as cryptographically insecure. Should confirm that it’s not being used in a way that this would be required.
  • Perhaps interesting in the context of a hash DoS attack, though this is a client lib so really no big deal even if this actually would be possible.
  • Seems to be used by implementation of CaseInsensitiveASCIIString
    • Does appear to be used on untrusted data (URLs, maybe header data?) but not obvious to me if the hashing comes into play
• What’s used for the boundary value in HTTPBodyStream.swift? If content in a multipart POST comes from different sources and some piece of content can spoof a valid boundary then that could be problematic.
  • Ahh, looks like it’s a UUID:
  • Not sure how random UUIDs generated in this way really are.
    • They seem to come from RFC 4122 version 4 which seems to suggest they aren’t necessarily based on a secure source of randomness
  • You could consider using a better source of randomness, assuming a UUID isn’t sufficiently random.

---

[lilyball]

Thanks! A lot of the issues you bring up are actually handled by URLSession and not by PMHTTP. I'll respond to each in turn.

Does it follow server-side redirects? Is this configurable?

By default, yes. As you highlighted, there is an option to turn that off. This matches the default behavior of Apple's URLSession (which follows redirects unless you implement a particular delegate method and tell it not to).

CR/LF injection in headers?

URLSession handles the actual HTTP exchange and its implementation handles headers exactly as specced in the relevant RFCs. I'm not sure offhand exactly what URLSession will do if I try and send a request with CR/LFs in the headers, most likely just strip them out, and of course headers in the response will be parsed correctly.

(Or in URL paths too for that matter)

Apple's URL type does not allow illegal characters in URLs. CR/LFs are illegal characters in URLs. If you try and pass a string containing CR/LFs to any of the request functions (such as request(GET:parameters:) it will return nil.

Seems to be handled appropriately by the quotedString function in HTTPBodyStream.swift at least.

HTTPBodyStream is the only part of PMHTTP that has to care about the actual representation of headers on the wire, as it has to serialize headers out as part of the multipart/mixed body, but as you pointed out it does escape user input appropriately here.

What happens if cert validation fails? (Does it fail secure by default?)

The default behavior for URLSession is to fail the request if cert validation fails. PMHTTP offers a way to customize this behavior using HTTPManager.sessionLevelAuthenticationHandler, but the default behavior if this is not specified is .performDefaultHandling, which means to use URLSession's built-in behavior, which is to validate the certificate and fail the request if validation fails.

Though I wonder if it makes sense to explicitly support HSTS or anything else, in any way?

Interesting suggestion. Rather complicated to do though at the library level, as it requires long-term storage of data, plus ideally supporting the HSTS preload list. This is normally only something that web browsers do.

Some cookie-related code in HTTPManagerRequest.swift

Not really. It provides access to a couple of URLRequest properties for controlling cookie support in URLSession (enabling cookies in general, and mainDocumentURL which affects URLSession's handling of document cookies). The only real custom Cookie-related code is in HTTPHeaders.addValue(_:forHeaderField:) as you pointed out, where it picks a different field delimiter based on whether the header is the Cookie header or not.

...what happens if the oldValue cookie string actually has a semicolon in it?

Then you end up with a broken cookie header. This API just deals with string values, not structured cookies, so it can't keep you from shooting yourself in the foot by putting a badly-formatted cookie in there. But you can do that anyway by simply assigning the bad string to the cookie header instead of using addValue(_:forHeaderField:), that method is just trying to be helpful by acknowledging that Cookie is a special header that separates multiple values in a different way than all other headers.

Re caching -- it is conceivable to mess this up, you can imagine if a cached HTTP resource could somehow be retrieved on a subsequent HTTPS request to the same origin / path.

Caching is handled by URLSession. The only caching-related code in PMHTTP is where we potentially override it to not cache a response when it might otherwise if we think it's a JSON response that didn't include any explicit cache control headers. As for actually using a cached response to fulfill a request, that's entirely up to URLSession.

Search for “unsafe”

Most of the "unsafe" is just a micro-optimization to skip an unnecessary typecheck when invoking NSObject.copy() or NSObject.mutableCopy() as those methods are declared as returning Any but we actually know the concrete type they return.

There is one kind-of sketchy use of "unsafe", which is all of the HTTPManagerRequest subclasses have an init(__copyOfRequest:) method that's public due to limitations in Swift (hence the __ prefix, to indicate that this should not be used) and uses unsafeDowncast(_:to:) to skip a type-check on the input parameter, but if someone does in fact call that on a subclass with the wrong request type as an input then they're violating the API contract (in particular the point where they're not supposed to call that at all) and this would be a programmer error. All of the internal use of that API are guaranteed correct.

There's also some bitcasting between two enum types that are guaranteed to have identical representation. And there's a bit of use of manually-allocated heap buffers for reading data from streams but it's all appropriately length-checked.

SipHash implementation reports itself as cryptographically insecure. Should confirm that it’s not being used in a way that this would be required.

It's only being used to provide the hash value for CaseInsensitiveASCIIString, and the only times we actually place those into a hashed container, we do so with statically-known strings (user input is only wrapped in CaseInsensitiveASCIIString in order to test against the set of statically-known strings). The other uses of CaseInsensitiveASCIIString are for comparison and don't involve hashing.

What’s used for the boundary value in HTTPBodyStream.swift?

As documented in the code you linked, it's a known prefix coupled with a UUID, which matches what WebKit itself does. The UUID is generated using Apple's NSUUID. Apple does not provide the source for this, but it's the standard way to work with UUIDs in all Cocoa code and is presumably trustworthy. Under the hood it appears to use the C uuid_generate_random() function, which itself is documented as using /dev/urandom as its source of random data.

[lilyball]

I'm going to go ahead and close this. Feel free to reopen if you have any more concerns.