ForceTLS is a mechanism to allow sites to mark domains under their control as "secure-only", so that all future requests sent by the browser for resources in those domains will be sent using SSL/TLS.

We are still awaiting a stable specification to work from. The document linked to from the crypto.stanford.edu URL is out of date. It describes a model that uses specially marked cookies to communicate ForceTLS policy, while the current thinking is to utilize a custom HTTP header to do so.

There's a working up to date proof-of-concept here: https://addons.mozilla.org/en-US/firefox/addon/12714 And respective documentation for how it works is here: http://forcetls.sidstamm.com

=JeffH posted v5 of the spec draft here: http://lists.w3.org/Archives/Public/www-archive/2009Sep/att-0051/draft-hodges-strict-transport-sec-05.plain.html The project and header are now called Strict-Transport-Security, or STS.

Adds support for STS header reading and upgrading of HTTP channels, tied into the permissions manager.

Sid, there are some subtle errors in your parser. You might be interested in the test cases here: http://src.chromium.org/viewvc/chrome/trunk/src/net/base/strict_transport_security_state_unittest.cc?view=markup

Heh, "subtle"? I flat out don't support the implicit LWS for the header -- nothing subtle about breaking all the time. I also don't support random different case on the letters, but I didn't see that as required in the spec. Thanks for pointing these out in the preliminary patch Adam, I'll fix it Monday when I add some unit tests.

(In reply to comment #6) > I also don't support random > different case on the letters, but I didn't see that as required in the spec. When you see a "foo" production in one of these grammars, that means case-insensitive. There's an RFC somewhere that explains what the grammar syntax means, but I can't put my fingers on it at the moment.

Improved parsing (allows LWS and arbitrary capitalization). Unit tests on their way.

Update to the previous patch uploaded: - Fixed some parser bugs - Parser has unit tests in /netwerk/test/TestSTS.cpp - STS is now disabled when the browser is in private browsing mode (to avoid using STS state as a side-channel cookie)

+static nsresult +ParseStrictTransportHeader(nsACString& aString, PRInt64 &maxage, PRBool &includeSubDomains) You probably don't want to define static functions in header files. You'll end up with a ton of duplicate code in your binary. My guess is you'd rather omit the "static" key word here. + TestSTS.cpp I didn't see TestSTS in the patch. We want to be sure not to record an STS header that we get over a "broken" HTTPS connection. It's possible your patch already does this, but I didn't understand how. Also, I didn't see the part where the STS pref prevents the user from overriding HTTPS certificate errors. Maybe that's work for a future patch?

Yes, I forgot to put the test cpp file in the bug. I caught that last night and intended to upload a new patch, but I ran into a few build errors that I've had trouble reproducing. The reason I had that function defined as static was so it would link properly with the unit test (or at least the linker would be satisfied). The unit tests in netwerk are set up kind of funny and I've had a bit of trouble trying to get them to work in general (Bug 529330). Omitting the static keyword hid the function from the unit test, and linking to the object where it was proved to be near impossible. I've put the parser in its own file, but it remains static. Hopefully that will work well enough (since it won't be referenced by any .h files). I'm currently in the process of figuring out why it fails to build on the try servers but builds and runs fine on my own box. When I get that sorted out I'll put up a new patch. Regarding "broken" connections -- yes, I didn't work that in there. I'm still trying to figure out the best way to do this. Regarding cert overrides -- this is future work, and will be part of the UI patch I write on top of this one to let users add/remove sites from the STS hosts list.

Awesome. Thanks Sid. :)

This one has working unit tests. I re-wrote the parser to depend less on internal stuff, and it solved the linking/try-server problems. The patch still needs to ignore STS headers on untrusted (self-signed) connections, and to cancel loads when the secure channel is not trustworthy.

There are a bunch of changes in this patch from the last one. (1) The parser is now an XPCOM service, and there are a few other things in the service for determining if something is an STS host, etc. (2) STS headers are ignored if there are any certificate errors on the transport layer (this is the main reason to push the STS stuff into an XPCOM service, so we could get at the required interfaces to determine if there are cert errors) (3) The STS-ness of a host (flag whether or not it is an STS host) is threaded through network and into nsNSSIOLayer (PSM) so that the certificate can be verified and NOT overridden if the target host is an STS one. This is a fairly messy way to kill a connection to an STS host when there is a cert error, but it appears to be the only way to avoid sending HTTP requests first. This is very important since we don't want to leak cookies and stuff on an untrusted channel. The flag for this takes the following path: nsHttpChannel::AsyncOpen() -> nsHttpConnection::OnSocketWritable() -> nsISSLSocketControl::SetStrictTransportState() -> nsNSSIOLayer.cpp:nsNSSBadCertHandler() It's a bit convoluted because it jumps through XPCOM and out of the main thread into the SSL thread. For example, I have to set the flag in securityInfo from OnSocketWritable() because that's the first place in nsHttpConnection that I could find where securityInfo was not null. (4) Unit tests were updated to use the XPCOM service for parse testing. Although this is spec-complete, it's not a complete feature. Currently when a bad cert is used for an STS host connection, the connection is just dropped. There's no warning dialog, no "add cert exception" dialog, nothing. It looks as if the new URL just stopped, and never replaced the old page. Here's a list of todo items I still want to implement: (1) When STS causes a top-level page load to cancel, show an error page like the current cert error page, but remove the ability to add a cert exception. (2) Update the site permissions UI to allow users to set a host's STS state without requiring the HTTP header. This will also help users remove STS permissions. I think I need to roll the first todo item into this patch since right now it's not clear why the blocked connection stops loading. The second one is more of an enhancement, and there will most likely be other UI enhancements too, so I'm going to hold off on that and maybe open another bug for it.

> (1) When STS causes a top-level page load to cancel, show an error page like > the current cert error page, but remove the ability to add a cert exception. That sounds reasonable. The UI Chrome uses for this is similar to the "host not found" and other network error messages rather than the certificate error messages. In Firefox, these two are more similar, so that probably doesn't matter too much.

Cleaned some redundant code, and added a UI tweak to the certificate error dialog so that users cannot add certificate error overrides for STS hosts. Requesting review from Kai since I touched a bunch of security/manager stuff.

Updated spec: http://lists.w3.org/Archives/Public/www-archive/2009Dec/att-0048/draft-hodges-strict-transport-sec-06.plain.html

Sid, drop me r?, I'll check this.

Comment on attachment 417805 [details] [diff] [review] Core STS Support (v1) >+++ b/netwerk/base/public/nsIStrictTransportSecurityService.idl Tue Dec 15 14:51:39 2009 -0800 >+ void parseStsHeader(in string aHeader, >+ out PRInt64 maxage, >+ out PRBool includeSubdomains); >+ >+ void setStsState(in nsIURI aURI, >+ in PRInt64 maxage, >+ in PRBool includeSubDomains); I understand you want this for the parser test, but would be better if you have a single method (say, called processSTSHeader) that would just take the header and setup the service internal state accordingly. You may probably return another argument about correctness of the header or just use a specific error to indicate it. That approach is better in future compatibility with the spec. I don't like having fields which count/names/meaning may change in the method signature. The class name should be "nsStrictTransportSecurityService", but it's a detail. >+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp Tue Dec 15 14:51:39 2009 -0800 > nsHttpChannel::Connect(PRBool firstTime) > { >+ if (!gHttpHandler->InPrivateBrowsingMode()) { >+ // enforce Strict-Transport-Security >+ PRBool isStsHost = PR_FALSE; >+ rv = GetSTSService()->IsStsURI(mURI, &isStsHost); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // if we're not using SSL and either the exact host matches or a >+ // superdomain wants to force HTTPS.... >+ PRBool usingSSL = PR_FALSE; >+ rv = mURI->SchemeIs("https", &usingSSL); >+ NS_ENSURE_SUCCESS(rv,rv); >+ >+ if (!usingSSL && isStsHost) { >+ LOG(("nsHttpChannel::Connect() STS permissions found\n")); >+ rv = DoUpgradeChannelToHttps(); >+ NS_ENSURE_SUCCESS(rv,rv); >+ return NS_OK; >+ } >+ } >+ I'd rather see this branched this way: if (!InPBMode) if (!usingSSL) if (stsService->IsSTSURI) Unnecessary permission manager usage could involve time regressions. >+nsresult >+nsHttpChannel::ProcessSTSHeader() >+ rv = GetSTSService()->ParseStsHeader(stsHeader.get(), &maxage, &includeSubDomains); >+ NS_ENSURE_SUCCESS(rv, rv); If you fail to parse the header you break the load. According to the draft only thing you have to do in that case is to ignore it. > nsresult >+nsHttpChannel::DoUpgradeChannelToHttps() There is no need for so much logging in this function. It is copy of an existing stable code. Just an info we do the replacement is enough here. >@@ -4422,16 +4590,25 @@ nsHttpChannel::AsyncOpen(nsIStreamListen >+ PRBool isSTSHost; >+ NS_ENSURE_SUCCESS(GetSTSService()->IsStsURI(mURI, &isSTSHost), NS_ERROR_FAILURE); Please go through rv = GetSTS...; if (NS_FAILED(rv)) return rv; >+++ b/netwerk/protocol/http/src/nsHttpChannel.h Tue Dec 15 14:51:39 2009 -0800 >+ // accessor for the permission manager (that laziliy inits the field) >+ nsCOMPtr<nsIStrictTransportSecurityService> GetSTSService(); Declare it like nsIStrictTransportSecurityService* GetSTSService(), there is no need to go through com ptr. >+ nsresult DoUpgradeChannelToHttps(); Maybe better would be DoRedirectChannelToHttps() ? >+++ b/netwerk/protocol/http/src/nsHttpConnection.cpp Tue Dec 15 14:51:39 2009 -0800 >+ , mSTSIsSet(PR_FALSE) Would be better to name this mWasStrictTransportSecuritySet, it better indicates what it means. >@@ -133,16 +135,19 @@ nsHttpConnection::Activate(nsAHttpTransa >+ mStrictTransportSecurityEnabled = (caps & NS_HTTP_STRICT_TRANSPORT_SECURITY) > 0; Nicer is != 0. > nsHttpConnection::OnSocketWritable() > { > LOG(("nsHttpConnection::OnSocketWritable [this=%x]\n", this)); > > nsresult rv; > PRUint32 n; > PRBool again = PR_TRUE; > >+ // Set a hint for PSM if the connection should be subject to >+ // Strict-Transport-Security. This is done here because this is the >+ // earliest point in the connection where the securityInfo is available. >+ if (!mSTSIsSet && mConnInfo->UsingSSL()) { >+ nsCOMPtr<nsISupports> securityInfo; >+ nsresult rv = mSocketTransport->GetSecurityInfo(getter_AddRefs(securityInfo)); >+ if (NS_SUCCEEDED(rv)) { >+ nsCOMPtr<nsISSLSocketControl> ssl = do_QueryInterface(securityInfo, &rv); >+ if (NS_SUCCEEDED(rv)) { >+ ssl->SetStrictTransportState(mStrictTransportSecurityEnabled); >+ mSTSIsSet = PR_TRUE; >+ LOG(("***** SET STS [this=%x mSTSEnabled=%d]\n", >+ this, mStrictTransportSecurityEnabled)); >+ } >+ } >+ } >+ I'll try to think of a better solution for this. This is quit ugly, but I don't have a better idea atm. At least you should set mSTSIsSet to TRUE after you find the security info to bypass the most of the code that would be called unnecessarily when mStrictTransportSecurityEnabled would be PR_FALSE. You actually don't need to do this at all in that case. >+++ b/netwerk/protocol/http/src/nsHttpHandler.cpp Tue Dec 15 14:51:39 2009 -0800 >@@ -1733,16 +1740,20 @@ nsHttpHandler::Observe(nsISupports *subj >+ else if (strcmp(topic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) { >+ mInPrivateBrowsingMode = (strcmp(NS_ConvertUTF16toUTF8(data).get(), >+ NS_PRIVATE_BROWSING_ENTER) == 0); >+ } Please be exact on testing the data argument. There are NS_PRIVATE_BROWSING_ENTER and NS_PRIVATE_BROWSING_LEAVE for this. I expect this all code is just for optimization as the pbm service is in JS, right? >+++ b/security/manager/boot/src/nsStrictTransportSecurityService.cpp Tue Dec 15 >+//#include "nspr.h" Don't leave commented headers. >+NS_IMETHODIMP >+nsStrictTransportSecurityService::RemoveStsState(nsIURI* aURI) >+{ >+ nsresult rv; >+ >+ nsCAutoString spec; >+ rv = aURI->GetSpec(spec); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ sPermMgr->Remove(spec, STS_PERMISSION); >+ STSLOG(("STS: deleted maxage permission\n")); >+ >+ sPermMgr->Remove(spec, STS_SUBDOMAIN_PERMISSION); >+ STSLOG(("STS: deleted subdomains permission\n")); >+ >+ return NS_OK; >+} Permission manager is using the host portion of the uri and not the whole spec. Clone this code: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#900 >+NS_IMETHODIMP >+nsStrictTransportSecurityService::ParseStsHeader(const char* aHeader, According the draft ABNF the header may also be in form: includeSubDomains;max-age=1000 This form is not supported by your parser. I would use a tokenizer loop (just my opinion). >+// Verify the trustworthiness of the channel (are there any cert errors?) >+NS_IMETHODIMP >+nsStrictTransportSecurityService::StsShouldBlockLoad(nsIHttpChannel* aChannel, >+ nsISupports* aSecurityInfo, >+ PRBool* aResult) I don't see a reason this have to be an interface method. Move its code to nsHttpChannel. Btw, you are not filling *aResult with tlsIsBroken. >+++ b/security/manager/boot/src/nsStrictTransportSecurityService.h Tue Dec 15 >+ >+nsCOMPtr<nsIPermissionManager> sPermMgr; Call this mPermMgr, it's a member, not a static var. >+++ b/security/manager/ssl/public/nsISSLCertErrorDialog.idl Tue Dec 15 14:51:39 2009 -0800 Why are you changing this interface's ID? >+++ b/security/manager/ssl/src/nsNSSIOLayer.cpp Tue Dec 15 14:51:39 2009 -0800 >@@ -3328,42 +3344,61 @@ nsNSSBadCertHandler(void *arg, PRFileDes >- PRUint32 overrideBits = 0; >+ if (strictTransportSecurityEnabled && >+ (status->mIsUntrusted || >+ status->mIsDomainMismatch || >+ status->mIsNotValidAtThisTime)) >+ { This condition is wrong and you don't need to test for mIsXXX memeber, here you will for sure have at least one of them always set. The condition should be: if (!strictTransportSecurityEnabled) { run the original code with override service } else { report error, sts is on and there is a cert error; } You must not remember an STS host if the host is an IP address (probably use PR_StringToNetAddr to check?). Include an automated test for this, use mochitest, feel free to ask me how to do that. You call GetSTSService()->IsStsURI() 3 times. It might make sense to cache it somewhere...

And I forget two more points: 1. GetSTSService() should be member of nsHttpHandler as IOService is. 2. The 'ugly' code with nsNSSSocketInfo::mStrictTransportSecurityState is probably wrong. I'm not sure we set its value sooner we need it in the bad cert handler. You should first have a good test for this (next time). I'll check this is ok or not, and let you know asap. We'll probably need some callback or whatever mechanism for things like these (as we were talking about that by email).

(In reply to comment #19) > I understand you want this for the parser test, but would be better if you have > a single method (say, called processSTSHeader) that would just take the header > and setup the service internal state accordingly. You may probably return > another argument about correctness of the header or just use a specific error > to indicate it. > > That approach is better in future compatibility with the spec. I don't like > having fields which count/names/meaning may change in the method signature. Good point. I merged the two into ProcessSTSHeader, and modified my binary unit tests appropriately. Unfortunately I can't easily check if the max-age was read properly (only that it was valid or invalid), but I should be able to test this in a mochitest when we figure out where to put them. > Unnecessary permission manager usage could involve time regressions. Good call. Fixed. > If you fail to parse the header you break the load. According to the draft > only thing you have to do in that case is to ignore it. Hm, the spec changed since I uploaded the patch. Fixed. > I'll try to think of a better solution for this. This is quit ugly, but I > don't have a better idea atm. Keep thinking. I made it a little better, but didn't change a whole lot. > At least you should set mSTSIsSet to TRUE after you find the security info to > bypass the most of the code that would be called unnecessarily when > mStrictTransportSecurityEnabled would be PR_FALSE. You actually don't need to > do this at all in that case. Good call. Done. > According the draft ABNF the header may also be in form: > includeSubDomains;max-age=1000 Again, changed since I uploaded the patch. Bah. Updated and fixed unit tests accordingly. > Why are you changing this interface's ID? Hm, probably left over from a previous incarnation of the patch. Reverted. > You must not remember an STS host if the host is an IP address (probably use > PR_StringToNetAddr to check?). Yes, thanks. Added the check to skip if it's an IP address. Thanks for the comments, here's another round. I look forward to your thoughts on making the ugly code prettier, and where to put mochitests for this.

Comment on attachment 425583 [details] [diff] [review] Core STS Support (v2) >diff -r 2c939a8b03c3 netwerk/protocol/http/src/nsHttpHandler.cpp >+nsIStrictTransportSecurityService* >+nsHttpHandler::GetSTSService() >+{ >+ if (!mSTSService) >+ mSTSService = do_GetService(NS_STSSERVICE_CONTRACTID); >+ return mSTSService.get(); >+} >+ No need for .get() >diff -r 2c939a8b03c3 security/manager/boot/src/nsStrictTransportSecurityService.cpp >+nsStrictTransportSecurityService::ProcessStsHeader(nsIURI* aSourceURI, >+ const char* aHeader) >+ char* header = (char*) asHeader.get(); Rather use BeginReading() instead of get(), I previously didn't notice. >+ if (!PL_strncasecmp(directive, "max-age", 7)) { >+ // skip directive name >+ directive += 7; >+ // skip leading whitespace >+ directive = (char*) NS_strspnp(" \t", directive); >+ STS_PARSER_FAIL_IF(*directive != '=', >+ ("No equal sign found in max-age directive\n")); Is there really allowed a white space between 'max-age' and '=' char? >+// Verify the trustworthiness of the channel (are there any cert errors?) >+NS_IMETHODIMP >+nsStrictTransportSecurityService::StsShouldBlockLoad(nsIHttpChannel* aChannel, >+ nsISupports* aSecurityInfo, >+ PRBool* aResult) You haven't addressed: I don't see a reason this have to be an interface method. Move its code to nsHttpChannel. Btw, you are not filling *aResult with tlsIsBroken. Also, there is a wrong indention in this method, use 2 spaces as in the rest of the file. If you want to leave it as an interface method, it's up to you, however, the method should have a better name, something like "ShouldIgnoreSTSHeader" or something, if that was what the method did; the current name is misleading. >diff -r 2c939a8b03c3 security/manager/ssl/src/nsNSSIOLayer.cpp >+ } else { >+ PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("Strict-Transport-Security is violated: untrusted transport layer\n")); > } According to article 7.3 of the draft (as I understand) you have to close the connection unconditionally in this state. It means you have to return SECFailure here that will cause the connection to break (I didn't say it clearly last time). See comments in bug 482870 for progress on the 'ugly code'. For now, let's put mochitest for this to security/manager/ssl/tests/mochitest/

Thanks for the re-review, Honza! Your thoughts are very helpful. Here are some replies. I'm still working on adding more tests to the patch, and will upload it to the bug when I've got those done. (In reply to comment #22) > No need for .get() Nice, didn't know it would automatically convert it for me! > Is there really allowed a white space between 'max-age' and '=' char? Yes, unfortunately. The spec defines "max-age" and "=" to be separate literals, and RFC 2616 told me there's implied OWS between all separate literals in the ABNF. RFC 2616 Sec 2.1 says: > implied *LWS > The grammar described by this specification is word-based. Except > where noted otherwise, linear white space (LWS) can be included > between any two adjacent words (token or quoted-string), and > between adjacent words and separators, without changing the > interpretation of a field. > You haven't addressed: I don't see a reason this have to be an interface > method. Move its code to > nsHttpChannel. Sorry, I must have forgotten to include a comment about this. I tried putting it in nsHttpChannel.cpp, but the interfaces nsISSLStatus and nsISSLStatusProvider (.h) are not available from within netwerk. I believe this is the main reason I have an nsIStrictTransportSecurityService. If you know of a way that I can get those interfaces into netwerk/protocol/http/src/nsHttpChannel.cpp, I'll gladly move it. > Btw, you are not filling *aResult with tlsIsBroken. Yeah, I must have totally missed this from the last review, sorry. > If you want to leave it as an interface method, it's up to you, however, the > method should have a better name, something like "ShouldIgnoreSTSHeader" or > something, if that was what the method did; the current name is misleading. The name evolved out of when it was used for more than just this before... I'll fix it to be clearer. > >diff -r 2c939a8b03c3 security/manager/ssl/src/nsNSSIOLayer.cpp > >+ } else { > >+ PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("Strict-Transport-Security is violated: untrusted transport layer\n")); > > } > > According to article 7.3 of the draft (as I understand) you have to close the > connection unconditionally in this state. It means you have to return > SECFailure here that will cause the connection to break (I didn't say it > clearly last time). That should be what happens, if I am following the code right: the override service doesn't get checked if the host is an STS host. The only way this method returns SECSuccess is if the override service has an override for the given cert (and I'm skipping that logic when a host is an STS host). It's important that there is some sort of error message so we can tell that the broken connection isn't just a bug, but actually intended to be closed. Furthermore, I had to modify the error message so the user can't add a cert override for the STS host too (see browser/components/certerror/content/aboutCertError.xhtml in this patch), so it's important that the rest of the method completes and then returns SECFailure at the end via cancel_and_failure() after setting up the error page. > See comments in bug 482870 for progress on the 'ugly code'. Thanks for working on this. It would be *really* cool if we could expose some sort of observer notification that fires when the SSL handshake is done, but before any HTTP traffic hits the wire. This way both STS and add-ons could leverage the SSL information before any HTTP data happens. That's a pretty lofty goal though, considering it spans multiple threads and would involve making an asynchronous feature synchronous for a little while as the observers are notified. > For now, let's put mochitest for this to security/manager/ssl/tests/mochitest/ Ok. I'll start plugging away at mochitests.

(In reply to comment #23) > > According to article 7.3 of the draft (as I understand) you have to close the > > connection unconditionally in this state. It means you have to return > > SECFailure here that will cause the connection to break (I didn't say it > > clearly last time). > > That should be what happens, if I am following the code right: Yes, sorry, you are right. Seems to be a good way that you have chosen.

CC'ing Ehsan since we'll have to be sure STS complies with private browsing mode (when the STS state is set by the web site).

I ran into a little road-block in implementing the mochitests. Here's my goal: 1. Set up three iframes 2. Load https://example.com/... in the first iframe (sets the STS state of example.com) 3. When that's done, load http://example.com in the second iframe 4. load http://test1.example.com in the third iframe. 5. Check if the second and third iframes were upgraded to https. My test breaks when trying to load the second and third iframes: an error is reported in one frame saying "unable to connect" and in the other "server not found." Reloading the individual iframes succeeds. A very similar test done on my local host's webserver (not mochitest) also works fine right away with no frame-reloading needed. I'm going to hunt around a bit looking for a way to get this working in mochitest unless someone has another idea of a good way to structure a mochitest.

(In reply to comment #23) > > Is there really allowed a white space between 'max-age' and '=' char? > > Yes, unfortunately. The spec defines "max-age" and "=" to be separate > literals, and RFC 2616 told me there's implied OWS between all separate > literals in the ABNF. HTTP-bis is more explicit on this point, probably best to align with it.

Good call, actually the spec seems to have been updated to explicitly state where OWS can be in the ABNF (and point to the HTTP-bis ABNF stuff). There is not OWS between 'max-age' and '=' in the STS spec, so there should be no whitespace allowed there. I will fix that!

(In reply to comment #28) > Good call, actually the spec seems to have been updated to explicitly state > where OWS can be in the ABNF (and point to the HTTP-bis ABNF stuff). There is > not OWS between 'max-age' and '=' in the STS spec, so there should be no > whitespace allowed there. I will fix that! Well, in reviewing this thread, as well as draft-hodges-strict-transport-sec-06.plain.html <http://kingsmountain.com/draft-hodges-strict-transport-sec-06.plain.html>, it seems to me that I messed up in the new (in -06) ABNF by not putting explicit OWS's in the maxAge production. IMV, they ought to be there so that the parsing of the header is as legitimately forgiving as possible. e.g... max-age=0 max-age =0 max-age= 0 max-age = 0 ..all ought to parse our equivalently it seems to me. =JeffH

Patch with non-working mochitests. Waldo and I are trying to figure out why the tests don't work in mochitest-land, but work in the "wild." Still no private browsing support, but it's coming!

I forked proper private mode into bug 557598 since we should be able to move forward not-supporting private mode for STS, and then add private mode support later. (Also, it's going to be a lot of work to add this to STS). The patch in this bug will make the STS data store read-only when the browser is in private mode. Bug 557598 should make it more robust and useful in private mode.

STS data is private, and should be clearable by the user via the "Clear Recent History..." dialog. I'm going to lump STS data into the "site prefs" bucket in the "Clear Recent History..." dialog. We don't record when the STS data is encountered, so clearing the "last hour" is not possible. I think it is appropriate to only clear STS data (and all of it) when "Everything" is selected for the time range.

I pulled the mochitests out of this patch because they're in a state of flux while this patch really isn't. I also fixed a few minor bugs in it, such as the bits that completely disable STS when in private browsing (instead of just making it read-only).

The mochitests rewritten to open new windows and use postmessage to communicate with the main test file. These still don't work right, and I'm not sure why. I've debugged the code in the STS Core patch and everything seems fine, but the mochitest HTTPS server isn't being found *only during the STS redirects* for some reason. I've tested my patch manually with an STS server and it works, but I just can't automate those tests. Honza, could you take a look at this (used with the v3 patch) and maybe suggest a course of action? I'd like to try and get this into the tree soon, but am hung up on writing functioning mochitests.

This is an update to both your patches to allow the mochitests to work. HttpHandler::NewChannel doesn't pass correctly the proxy info, that's why the connection fails. We may suffer from the same issue also on other places (fallback e.g.). I'll check it and submit a new bug for that if necessary. Your tests also have to check when permission doesn't apply to sub-domains. To check that loading https: resources is not affected is also good. Also don't forget to remove the STS permissions, i.e. do a clean up of what your patch has persisted, the test must not influence other tests and must be runnable again it self.

Comment on attachment 446112 [details] [diff] [review] STS Mochitests The approach seems to be good, see the previous comment for feedback. Do you plan to have the core patch get reviewed (after the fix)? I think we can go with "the ugly code" as it is. To do this properly means to do more changes, let's do it in a different bug.

Thanks for the sage-like advice, Honza! Yes, I do plan to get this reviewed as-is once there's working and robust unit tests; I also think that fixing up the "ugly code" is a more architectural change than we should lump into this bug.

Okay, it's wrapped up into one patch, and the tests should be more robust. I check two STS headers (one with subdomains, one without) and clear any persistent state set by STS. These mochitests are, of course, in addition to the unit tests in netwerk/test/TestSTSParser.cpp.

Comment on attachment 447410 [details] [diff] [review] Core STS Support (v4) I can't find the mochitests in the patch. BTW: are you using mercurial patch queues to create your patches?

Ack, sorry. Forgot to do a qrefresh before making the patch. I am indeed using mercurial queues, but not making the patches that way (using hg qdiff). I made a couple of patches that way and decided it was a bad idea, so I've quit that and am back to piping qdiff output into a file.

I propose to clarify section 7.3 of the STS document. It currently says: "When connecting to a Known STS Server, the UA must terminate the connection with no user recourse if there are any errors (e.g. certificate errors), whether "warning" or "fatal" or any other error level, with the underlying secure transport." I propose to add another section: During the initial https connection to a STS server, the server certificate verification performed by a UA happens prior to receiving the http headers. If a user has configured the UA to continue the connection despite certificate errors, the UA must ignore any such configuration as soon as the server identifies itself as a STS server. Based on comment 23 in this bug I conclude we agree on this, but I'm worried that some readers of the spec might overlook this detail. Also, I'm worried that someone might be able to construct an attack based on the fact that the initial connection is not yet strict. I propose to be more restrictive for the initial connection if it involved errors and error overrides, and propose to add this: If an https connection to a not-yet-known-as-STS-server involves certificate errors and a user's decision to ignore certificate errors, and the server identifies itself as being a STS server, then the UA MUST show an error message and MUST NOT display the information received from the server.

I understand the previous comment 41 is offtopic for this bug. If you are in contact with the authors of the STS specification, could you please forward my proposal to them on my behalf? Thanks!

Here are some first comments (still reviewing): (a) Please check the license headers in your patch: - don't copy them from other files, but from the official license boilerplate location http://www.mozilla.org/MPL/boilerplate-1.1/ - don't say "Corporation" but say "Mozilla Foundation". (Gerv requested that during his lightning talk at the foundation) - update the year to the correct value. (b) > PRPackedBool mStrictTransportSecurityState; Optional rename request: Given it's a bool, I'd prefer a variable name that makes it more obvious what "true/false" means, like mUsesTransportSecurity. (c) + /* + nsresult RemoveStsState(nsIURI* aURI); + nsresult ProcessStsHeader(nsIURI* aSourceURI, + const char* aHeader); + nsresult IsStsHost(nsIURI* aURI, PRBool* aResult); + nsresult ShouldIgnoreStsHeader(nsIHttpChannel* aChannel, + nsISupports* aSecurityInfo, + PRBool* aResult); + */ I personally wouldn't mind this reminder comment, but in the past other reviewers have asked me to remove such dead code. I recommend to remove it prior to check in. One argument for removing is, such comments tend to not get updated when the underlying interface gets changed. (c) Not a request, just a thought, I wish the nsIPermissionManager would state more clearly in its interface that permissions aren't based on individual full URIs, but rather based related by hostnames, only. (d) +nsStrictTransportSecurityService::SetStsState(nsIURI* aSourceURI, +{ + mPermMgr->Add(aSourceURI, STS_PERMISSION, + (PRUint32) nsIPermissionManager::ALLOW_ACTION, + (PRUint32) nsIPermissionManager::EXPIRE_TIME, + expiretime); + + if (includeSubdomains) { + mPermMgr->Add(aSourceURI, STS_SUBDOMAIN_PERMISSION, + (PRUint32) nsIPermissionManager::ALLOW_ACTION, + (PRUint32) nsIPermissionManager::EXPIRE_TIME, + expiretime); + } A site could change it's rules and stop announcing the subdomain flag. Should you add else { // !includeSubdomains mPermMgr->Remove(); } ? (e) You don't have any calls RemoveStsState (except in the tests). Given that perm-mgr takes expiration time as input, it will probably remove the remembered STS flags automatically? But if I understand the specs correctly, a server is allowed to update the existing flags and the client is supposed to always use the freshest information sent by the server (see design decision note 3). Could a server ever decide it no longer wants to be an STS server? Does the specs allow for that, e.g. by sending the "Strict-Transport-Security:" header without any flags after the keyword? If the specs doesn't allow that yet, maybe it should be allowed? (f) It seems likely that most hosts will be STS hosts forever, which also means we'll be collecting a lot of data in permission manager over time. Should we try to save space? If the strings from #define STS_PERMISSION "sts/forcehttps" #define STS_SUBDOMAIN_PERMISSION "sts/includeSubdomains" are stored with each recorded host, maybe we want to use shorter strings? What about shortening strings to #define STS_PERMISSION "sts/use" #define STS_SUBDOMAIN_PERMISSION "sts/subs"

Thanks for the feedback, Kai! (In reply to comment #43) > Please check the license headers in your patch: Yes, will do, sorry. > (b) > > PRPackedBool mStrictTransportSecurityState; > Optional rename request: Given it's a bool, I'd prefer a variable name that > makes it more obvious what "true/false" means, like mUsesTransportSecurity. Great idea. > (c) > + /* > + nsresult RemoveStsState(nsIURI* aURI); > + nsresult ProcessStsHeader(nsIURI* aSourceURI, > + const char* aHeader); > + nsresult IsStsHost(nsIURI* aURI, PRBool* aResult); > + nsresult ShouldIgnoreStsHeader(nsIHttpChannel* aChannel, > + nsISupports* aSecurityInfo, > + PRBool* aResult); > + */ > > I personally wouldn't mind this reminder comment, but in the past other > reviewers have asked me to remove such dead code. I recommend to remove it > prior to check in. One argument for removing is, such comments tend to not get > updated when the underlying interface gets changed. I'll remove it. > (c) > Not a request, just a thought, I wish the nsIPermissionManager would state > more clearly in its interface that permissions aren't based on individual > full URIs, but rather based related by hostnames, only. Yeah, that would be nice. :) > A site could change it's rules and stop announcing the subdomain flag. > Should you add > else { // !includeSubdomains > mPermMgr->Remove(); > } Yes, indeed, I should! Good catch. > (e) > You don't have any calls RemoveStsState (except in the tests). I could remove it, but as soon as we add a UI, we'll want to give the users ability to remove STS state for hosts. > Given that perm-mgr takes expiration time as input, it will probably remove > the remembered STS flags automatically? Yes, for server-specified state, but if we ship with pre-included hosts or let users add some, they'll want to keep the STS data permanently but could change their minds. (Permission Manager also allows permanent permissions that won't expire). > But if I understand the specs correctly, a server is allowed to update the > existing flags and the client is supposed to always use the freshest > information sent by the server (see design decision note 3). > > Could a server ever decide it no longer wants to be an STS server? Does the > specs allow for that, e.g. by sending the "Strict-Transport-Security:" header > without any flags after the keyword? If the specs doesn't allow that yet, > maybe it should be allowed? This can be accomplished with "Strict-Transport-Security: max-age=0" > What about shortening strings to > #define STS_PERMISSION "sts/use" > #define STS_SUBDOMAIN_PERMISSION "sts/subs" Good idea! I think the short strings are sufficiently explanatory.

> > Could a server ever decide it no longer wants to be an STS server? ... > > This can be accomplished with "Strict-Transport-Security: max-age=0" Ok. In nsStrictTransportSecurityService::SetStsState your code will call mPermMgr->Add(now + zero) I propose to use if (maxage) { mPermMgr->Add() } else { mPermMgr->Remove() } > > > What about shortening strings to > > #define STS_PERMISSION "sts/use" > > #define STS_SUBDOMAIN_PERMISSION "sts/subs" > > Good idea! I think the short strings are sufficiently explanatory. I just looked up that "subs" is its own word in the english language (meaning replacement?). Maybe the even shorter "sts/sub" is better.

(In reply to comment #44) > > (e) > > You don't have any calls RemoveStsState (except in the tests). > > I could remove it, but as soon as we add a UI, we'll want to give the users > ability to remove STS state for hosts. That's a good argument for keeping it, thanks for explaining the motivation. (... still reviewing rest of patch)

Please clarify what kind of hostname do you expect. Is it Unicode encoded as UTF-8, or do you expect punycode? interface nsIStrictTransportSecurityService : nsISupports { ... PRBool isStsHost(in string aHost); } See http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsICertOverrideService.idl#72 for an example, where we make that clear. (I don't know which of both NS_NewURI expects, used by your implementation, you might want to research.) Once you are happy with the code, please spend some more work on improving the comments in the nsIStrictTransportSecurityService interface, a person wearing their super-reviewer hat will probably ask you to do that, too. You could clarify also that the implementation is expected to make decisions based on scheme and host contained in the URI (meaning it will ignore other portions as port and path).

I see you are not changing the nsCertOverrideService itself. Ideally, addons like https://addons.mozilla.org/en-US/firefox/addon/79787/ should not be able to add an exception anyway. If you're able to add an exception with that addon (see certificate manager, server's tab), then I propose the nsCertOverrideService implementation should query the nsIStrictTransportSecurityService and reject requests to store new exceptions for known STS hosts. You might say, even a stored exception won't help, because the http networking layer will cancel the connection anyway? And you might be right! If the connections really get blocked despite a stored override, please consider this proposal optional.

Architectural proposal: I'd prefer to split nsStrictTransportSecurityService::processStsHeader into two separate functions. I'd use a separate parseStsHeader function (which returns maxage, want-subdomain), and a addStsState function. You already needed an setStsState function in your implementation. I expect you'll need the latter function as soon as you write the UI, allowing the user to manually add hosts, and it should not be necessary to construct a header line for feeding into the service.

I think it's unfortunate that in private-browsing-mode we won't remember the server's STS request, and server content mistakes can still result in requests with session cookies being leaked over a plain http connection. Even though the server will redirect each request, the session data will already have been leaked. I understand the intent to not leave any traces, and not permanently remember the STS server. I propose to improve our logic in the following way (feel free to forward this proposal to a separate bug): - if we are in private browsing mode, and we see a new host, don't use EXPIRE_TIME, but use EXPIRE_SESSION (a quick glance at nsPermissionManager shows that it won't call "UpdateDB", which proably means, it will remembered in memory only) - have nsStrictTransportSecurityService remember the list of temporary overrides added during privacy mode. - when private browsing mode ends, remove all session-only STS hosts knowledge acquired during PBM

One more detail which I don't see addressed in the spec yet: port numbers! IMHO the primary key should not be "host", but "host:port". Also, what happens if the server choses to issue an redirect to a non-standard port number? The current code is hard-coded to always construct STS-host uris to the default port (-1 = 443). The current code will fail if a STS-host choses to redirect to https://www.host.com:444 The current code will do funky things if there are two separate http servers running on the same host, e.g. - http://host.com/ (default 80) and - http://host.com:8080 This configuration is often used on web servers, where the default port is the actual server, and the additional port is a web-site-maintenance interface. Let's say www.site.com is using port 80 for the public web and is used to redirect to 443. www.site.com is behind a firewall and won't allow connections to ports other than 80 and 443 from the outside world. www.site.com has a plain http maintenance interface on port 8080. I think with the current code, it's impossible to connect to http://www.site.com:8080, because our http engine will always catch that request and redirect to https://www.site.com:443/ If you want to keep it simple, and only address the standard configuration, my proposal is: - restrict the redirection to http requests that go to port 80

Comment on attachment 449100 [details] [diff] [review] Core STS Support (v4) r-, see my comments. Please explain if you decide not to address some of my requests, thanks. (Note to self: Have not yet reviewed nsHttpChannel::ProcessSTSHeader, nsHttpConnection::OnSocketWritable, nsStrictTransportSecurityService::ProcessStsHeader (which might be get split into two functions))

(In reply to comment #49) > Architectural proposal: > > I'd prefer to split nsStrictTransportSecurityService::processStsHeader > into two separate functions. > > I'd use a separate parseStsHeader function (which returns maxage, > want-subdomain), and a addStsState function. I had this set-up originally, but changed it. See comment 19 where Honza explains that having it encapsulated into one function will help preserve the interface with future changes to the spec (which will most likely be extended to have additional syntax). I'd like to leave this as one function for now.

(In reply to comment #50) > I think it's unfortunate that in private-browsing-mode we won't remember the > server's STS request, and server content mistakes can still result in requests > with session cookies being leaked over a plain http connection. Even though the > server will redirect each request, the session data will already have been > leaked. Yeah, this is a good point, and why I filed bug 557598. I was hoping to land the core functionality and let it bake while I worked on proper private-mode support. I don't think we should ship STS without proper private-mode support (which is why this bug is blocked by bug 557598), and I like the approach you mention. > I understand the intent to not leave any traces, and not permanently remember > the STS server. > > I propose to improve our logic in the following way (feel free to forward this > proposal to a separate bug): > - if we are in private browsing mode, and we see a new host, > don't use EXPIRE_TIME, but use EXPIRE_SESSION > (a quick glance at nsPermissionManager shows that it won't > call "UpdateDB", which proably means, it will remembered in memory only) > > - have nsStrictTransportSecurityService remember the list of > temporary overrides added during privacy mode. > > - when private browsing mode ends, > remove all session-only STS hosts knowledge acquired during PBM Maybe we can do this in bug 557598? I want to omit this part from the patch for this bug and implement that in the private mode support patch.

(In reply to comment #48) > You might say, even a stored exception won't help, because the http networking > layer will cancel the connection anyway? And you might be right! If the > connections really get blocked despite a stored override, please consider this > proposal optional. This is the way I see it. The cert override service is not queried for STS hosts, so even if there is a cert override, it's ignored. I agree that we might want to consider disallowing adding cert overrides for known-STS hosts even from add-ons, but I don't consider it part of the STS core and it's more of a tweak since the spec is not violated either way, so I'm going to skip this change for now.

Thanks again for the review comments, Kai. I've attached a new patch that should incorporate all your comments except for the ones I've replied to (resistantly) on the bug.

With respect to port numbers, I think that's not well covered in the spec. The way it works in chrome is that the STS state is for the whole host (this is important to protect secure cookies, which are shared by the whole host). For the redirect behavior: port numbers are preserved except port 80 is changed to port 443 because 80 is the default port for http.

Kai: thanks for reviewing this on my behalf, great comments. I didn't so far find time to do this my self.

Sid/Kai, Can I ask you to send your comments on the redirection and port handling behavior to the hasmat@ietf.org mailing list? If you'd rather not I can post it for you but I think you have outlined something that should be discussed publicly and at least clarified in the spec, or in some example section.

We've ported the spec to the IETF as an Internet-Draft, and are working towards spinning up a working group for it and other web-app-security specs in the IETF space. Here's a link that will yield the latest rev of the spec.. <http://tools.ietf.org/html/draft-hodges-strict-transport-sec> Here's a link to the mailing list this is being discussed on.. <http://tools.ietf.org/html/draft-hodges-strict-transport-sec> Thanks, =JeffH

please note the change log in the -02 spec. (see above post for link) as I was going through the spec, I added OWS to the max-age production because it seemed to me that not having it there was an oversight from a "usability" perspective. There may be other similar changes we might want to make to the STS header field ABNF as we finish up the spec -- we should discuss those on the hasmat@ list. Note also that we've updated the name of the spec to be "HTTP Strict Transport Security (HSTS)" because it is specific to HTTP, yet the "strict transport security" could be applicable to other protocols that run over TLS/SSL (or other secure transports). However, the STS header field remains named "strict-transport-security". =JeffH

Oh, the 4th para in my above post should have read.. Note also that we've updated the name of the spec to be "HTTP Strict Transport Security (HSTS)" because this spec is specific to HTTP. Note that the "strict transport security" /notion/ could be applicable to other protocols that run over TLS/SSL (or other secure transports).

Updated to address the port situation like Chrome (see comment 57).

While reviewing function nsHttpChannel::ProcessSTSHeader I had these comments: Optional Proposal: re comment 43 (b) You addressed my proposal to rename the bool variable (thanks), but we still have + attribute PRBool strictTransportState; If you want it to be consistent, you could rename the attribute to usesStrictTransportSecurity ------ Change Request: you have multiple occurrences of gHttpHandler->GetSTSService()-> I'm worried this may crash in OOM situations. I propose to use code like stss = gHttpHandler->GetSTSService(); if (stss) { .... } or NS_ENSURE_TRUE(stss, ...) You can simplify the code in ProcessSTSHeader(), where you have above call chain 3 times, and have the 2nd+3rd use "stss->" ------ Ok, you're aborting the connection, when we detect a bad cert and the host is an STS host... good. However, in ProcessSTSHeader() you say: + // Next, we need to check to see if this host is already an STS host: if + // so, it can't load over a broken connection (any cert errors). + // If it wasn't an STS host, errors are allowed, but no more STS processing + // will happen during the session. Your comment makes me think that it will be followed by code like if (wasAlreadySTSHost && tlsIsBroken) { abort-channel; } However, all you do is to derive the state "wasAlreadySTSHost", but you don't make any use of that information. I agree the abort is not necessary, because the nss-bad-cert-handler should catch it. However, I suggest: - remove the portion of the comment that proposes that the check-and-potential-abort will follow - add an NS_ASSERTION( ! (wasAlreadySTSHost && tlsIsBroken), "unexpected code path, should have been caught by nss-bad-cert-handler") (now proceeding to review nsHttpConnection::OnSocketWritable and nsStrictTransportSecurityService::ProcessStsHeader)

Function nsStrictTransportSecurityService::ProcessStsHeader makes me slightly nervous. You're scanning into PRInt64, so instead of sscanf you should use PR_sscanf. + // log unknown tokens, but don't fail (for backwards compatibility) You mean forward compatibility? You want to be compatible with future versions of the spec that may allow additional tokens after the max-age integer? Instead of using "max-age" and 7 (twice), please use: NS_NAMED_LITERAL_CSTRING(max_age_var, "max-age") and max_age_var.length() (not sure about the exact syntax, but this should be possible somehow) Same for includesubdomains / 17. + foundMaxAge = PR_TRUE; + + // skip max-age value and trailing whitespace + directive = (char*) NS_strspnp("0123456789 \t", directive); + + // log unknown tokens, but don't fail (for backwards compatibility) + if ((token = NS_strtok(" \t", &directive))) { + foundUnrecognizedTokens = PR_TRUE; + STSLOG(("Extra tokens in max-age after delta-seconds: %s %s\n", token, directive)); + } While I'm trying to understand the code, I'm writing my understanding down... Your above code would accept this line: Strict-Transport-Security: max-age=123abc or Strict-Transport-Security: max-age=123.45abc Is that intended or would you like to forbid the trailing abc / .45abc? (I personally don't mind). Now that you're done parsing the integer, you skip over the digits and following whitespace, and want to report anything that follows up to the next semicolon. Why do you try to tokenize the remainder? A directive like Strict-Transport-Security: max-age=123abc def ghi seems to result in this warning about extra tokens: abc def ghi which seems equivalent to simply print the remainder without tokenizing? Conclusions: - Your log output is irrelevant for optimized builds. You should avoid producing the tokens in optimized builds, given that they'll be thrown away anyway. - I'm slightly nervous your using NS_strtok to mutate the results of what you had received from an earlier call to NS_strtok, but well, you're code seems correct. - However, I'm not sure this is really necessary. Why waste time on tokenizing stuff we don't understand? Could you simply log the remainder instead? I'm also a bit nervous that you cast const away from the return value of NS_strspnp. Please do this: - change "char *directive" to "const char *directive" - don't mutate directive (i.e. don't use NS_strtok on directive) - remove the typecast (char*) Note, old-world casts should nowadays be completely avoided, even when there's a valid use. The preferred way is now to use TYPE const_cast<TYPE> (object) (search a reference for "const_cast") Also, your code will accept this: Strict-Transport-Security: max-age=15768000 ; includeSubDomainsOnlyIfWhatever If you're worried about forward compatibility, I think your code should make sure a delimiter follows after "includeSubDomains".

Reviewing OnSocketWriteable: I'm OK to leave the code as is, for now, but I think it should be improved in the future. Right now, you will add a little piece of overhead to every single SSL connection. It might not be noticed in performance runs, because most probably ignore SSL? I would be good if "find sts server, get state, remember flag" could be avoided, and only be excecuted on demand. ... After having talked with Sid on IRC, he will attempt to avoid the flag, declare the the STS-service thread-safe, and attempt to use a proxy for calls to the permission manager.

Comment on attachment 462896 [details] [diff] [review] HSTS Core Support v5.1 r-, but we're close. Thanks in advance for addressing my comments.

Thanks Kai. I think I addressed all your comments (and I got rid of that annoying status boolean by using a proxy). I'll post detailed responses to the comments shortly.

Great work! I have only one more nit, then you're ready to go. Sorry I didn't see it earlier. You shouldn't mutate the internal buffer of an nsCAutoString, the return of BeginReading should really be treated read-only, IMHO. Maybe I'm nuts and I shouldn't worry? Maybe, if you find a SR person who says your code this is fine, you can skip it. There's no need to use nsCAutoString if you're going to mutate the buffer, given your input is const char *, you should use PR_strdup(). Problem is you'll have to ensure to the result gets freed using NS_Free when you're done, and your function has many exit points. Here's my proposal: Rename nsStrictTransportSecurityService::ProcessStsHeader to nsStrictTransportSecurityService::ProcessStsHeaderMutating and change signature to take "char *aHeader" (not const) Remove the local variable "header" inside it. Create a new function with the old name: nsresult nsStrictTransportSecurityService::ProcessStsHeader (nsIURI* aSourceURI, const char* aHeader) // will mutate aHeader { char *header = PR_strdup() if (!header) return error; nsresult rv = ProcessStsHeaderMutating(uri, header); NS_Free(header); return rv } If you do the above, r=kaie

Thanks Kai. Adding your mutation suggestion... PR_Strdup was not declared anywhere convenient, so I wrote it inline with PR_Malloc + strcpy. The only thing this patch doesn't address has to do with private browsing: STS hosts, though in this patch not redirected to HTTPS during private browsing will still be canceled in the case of a cert error. This means that STS is not _completely_ off in private mode through this patch. I think this is minor and will be fixed by bug 557598 so I'm not going to address it in this patch. Flagging r+ via Kai's r=kaie in comment 69. Ready for your eyes, mrbkap!

(In reply to comment #70) > > Thanks Kai. Adding your mutation suggestion... PR_Strdup was not declared > anywhere convenient, so I wrote it inline with PR_Malloc + strcpy. It's PR_strdup (lowercase s). + strcpy(header, aHeader); + if (!header) return NS_ERROR_FAILURE; Please swap the lines (check before copy) and use error code NS_ERROR_OUT_OF_MEMORY Allocator and free must match. Either PR_Malloc and PR_Free or PR_strdup and NS_Free (I looked up which allocator PR_strdup uses internally)

Oops, my bad. Using NS_strdup to do the dirty work now, and returning the appropriate OOM error when !header (also using NS for both strdup and Free).

Comment on attachment 463301 [details] [diff] [review] HSTS Core Support (v5.3) Honza, could you give the tests a once-over? I think you've reviewed them before, so it should be quick.

(In reply to comment #73) > Honza, could you give the tests a once-over? I'll do it during weekend.

Is reviewing the tests a precondition to getting it landed? I'm ok with adding the tests to security/manager/tests, even if there hasn't been a full review of the latest tests yet. I'd ask that you do a tryserver build before landing, and make sure there aren't any failures introduced by your tests, though.

Comment on attachment 463301 [details] [diff] [review] HSTS Core Support (v5.3) >+++ b/netwerk/protocol/http/nsHttpChannel.cpp Thu Aug 05 14:25:35 2010 -0700 You are messing with indention: the first is 4 spaces, nested are 2 only. > nsHttpChannel::Connect(PRBool firstTime) >+ rv = gHttpHandler->GetSTSService()->IsStsURI(mURI, &isStsHost); You didn't address the change request from comment 64, which I'm also for. >+ rv = DoRedirectChannelToHttps(); IMPORTANT: We need to adapt your patch to use the new redirect API (bug 513086 and bug 546606). I'll start working on this adoption tomorrow morning (Monday). If anyone else is about to start working on this, please drop a note! >@@ -4469,16 +4657,25 @@ nsHttpChannel::AsyncOpen(nsIStreamListen >+ PRBool isSTSHost; >+ rv = gHttpHandler->GetSTSService()->IsStsURI(mURI, &isSTSHost); >+ if (NS_FAILED(rv)) >+ return rv; >+ You don't need this whole hunk, I think. >@@ -5672,16 +5869,17 @@ nsHttpChannel::IsFromCache(PRBool *value > } > >+ > //----------------------------------------------------------------------------- > // nsHttpChannel::nsIResumableChannel > //----------------------------------------------------------------------------- Please remove the extra new line >@@ -1799,16 +1814,23 @@ nsHttpHandler::Observe(nsISupports *subj >+ const char* switchType = NS_ConvertUTF16toUTF8(data).get(); >+ if (strcmp(switchType, NS_PRIVATE_BROWSING_ENTER) == 0) switchType has already been released in time you are testing it with strcmp. You have to use a construction NS_ConvertUTF16toUTF8 switchType(data); and then use switchType.get() to pass to strcmp. >+++ b/security/manager/boot/src/nsStrictTransportSecurityService.cpp Thu Aug 05 14:25:35 2010 -0700 >+NS_IMPL_ISUPPORTS1(nsStrictTransportSecurityService, nsIStrictTransportSecurityService) Shouldn't this be thread safe? You are doing do_GetService on a background thread, so you call AddRef()/Release() on it as well. I probably have seen a way to let the proxy manager do this on the target thread, but I cannot find an example. >+nsStrictTransportSecurityService::ShouldIgnoreStsHeader(nsIHttpChannel* aChannel, >+ nsISupports* aSecurityInfo, >+ PRBool* aResult) Why this method takes aChannel argument when it is unused? >+nsStrictTransportSecurityService::ProcessStsHeaderMutating(nsIURI* aSourceURI, >+ char* aHeader) >+ // copy string, we're going to mutate it. You probably don't need this comment. >+ else if (!PL_strncasecmp(directive, include_subd_var.get(), include_subd_var.Length())) { >+ includeSubdomains = PR_TRUE; >+ STSLOG(("STS: ProcessStrictTransportHeader: obtained subdomains status\n")); >+ >+ directive += include_subd_var.Length(); >+ // skip trailing whitespace >+ directive = NS_strspnp(" \t", directive); >+ >+ if (*directive != '\0') { >+ foundUnrecognizedTokens = PR_TRUE; >+ STSLOG(("Extra stuff after includesubdomains: %s\n", directive)); >+ } >+ } I don't think you have addressed last two paragraphs from comment 65. includeSubDomainsOnlyIfWhatever is parsed as includeSubDomains with a warning, although it was a completely different directive. Again! We need a C++ regexp API to do things like these properly! +++ b/netwerk/test/TestSTSParser.cpp Thu Aug 05 14:25:35 2010 -0700 >+ // these are weird tests, but are testing that some extended syntax is >+ // still allowed (but it is ignored) >+ rvs[20] = TestSuccess("max-age=100randomstuffhere", PR_TRUE, stss, pm); >+ rvs[21] = TestSuccess("max-age=100 includesubdomains", PR_TRUE, stss, pm); >+ rvs[22] = TestSuccess("max-age=100 bar foo", PR_TRUE, stss, pm); Please, add also test for 'max-age=100 ; includeSubDomainsSomeStuff' >+++ b/security/manager/ssl/src/nsNSSIOLayer.cpp Thu Aug 05 14:25:35 2010 -0700 >@@ -3397,42 +3398,72 @@ nsNSSBadCertHandler(void *arg, PRFileDes Just a side note: shouldn't bypass of cert overrides only apply to the http protocol? This will affect also smpt, pop, imap, snews, sftp and other protocols. We should check this is an http transfer, therefor the flag were walking through layers down here. But let's do this in a follow-up. >+++ b/security/manager/ssl/tests/mochitest/stricttransportsecurity/Makefile.in Thu Aug 05 14:25:35 2010 -0700 >+ subdom_bootstrap.html^headers^ \ >+ verify.sjs \ >+ test_stricttransportsecurity.html \ >+ $(NULL) >+ Please fix the indention. +++ b/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_stricttransportsecurity.html Thu Aug 05 14:25:35 2010 -0700 >+ var testframes = { >+ 'samedom': >+ {'url': "http://example.com" + STSPATH + "/verify.sjs", >+ 'expected': {'plain': 'SECURE', 'subdom': 'SECURE'}, >+ 'received': {'plain': null, 'subdom': null}}, You probably don't need 'received' objects ; removing them would also make the tests more readable. >+ if (result.length != 2) { >+ if(result[0] === "BOOTSTRAP") { Please be consistent with space between if and (, the same for for. We may need to ensure that the STS service is first time instantiated on the main thread, maybe somewhere in nsHttpHandler init or so, to ensure the permission manager is not refcounted on a different thread then the main one. Probably not needed when the service is instantiated only for http protocol as commented above.

- merge to m-c (apply over failed push of v5.3) - redirect to https is now async using the async redirect API - other comments remain unfixed - test passes

Thanks for the review Honza. I've implemented most all of your change requests, brought the patch up to date on mozilla-central, and folded your async redirect patch into this patch. I'm going to wait to obsolete your interdiff since I noticed there's an r? flag on it. Specific Replies: - I addressed the change in comment 64 with respect to the OOM situation. I misread it the first time, but think I got it all now. - Good catch on the unneeded hunk... there was some more around that hunk that was unnecessary, so I removed it. - the nsStrictTransportSecurityService in itself is threadsafe, but it relies upon the permission manager (without a proxy), which is not threadsafe, so I have not implemented the service as threadsafe. - I improved the parser to address the "includeSubdomainsFooBar" issue in comment 65. Also added a test for this. Thanks again!

(In reply to comment #78) > - the nsStrictTransportSecurityService in itself is threadsafe, but it relies > upon the permission manager (without a proxy), which is not threadsafe, so I > have not implemented the service as threadsafe. I believe you have to (just change the macro to NS_IMPL_THREADSAFE_ISUPPORTSn), because you move the STS service refcounter on more then a single thread. If you think not, then please check there are no thread-safety warnings (assertions) in the console while running your tests, but I believe you will find them. Thanks for the quick update, I'll get to this probably on Wednesday, hopefully sooner. PS: the interdiff should get reviewed from someone; specifically from Bjarne that co-implemented the new redirect API, so he is the best person to do it.

This patch goes on top of core support v6. (In reply to comment #79) > I believe you have to (just change the macro to NS_IMPL_THREADSAFE_ISUPPORTSn), > because you move the STS service refcounter on more then a single thread. If > you think not, then please check there are no thread-safety warnings > (assertions) in the console while running your tests, but I believe you will > find them. Talked to jst about this and he recommended also returning errors in the methods that are not threadsafe when they're called from non-main threads. Honza, could you take a peek when you look at the core patch again, and let me know if this is what you had in mind?

Sid, you need to: 1. make the AddRef and Release implementations thread safe with NS_IMPL_THREADSAFE_ISUPPORTS(...) 2. adding check methods are called on the main thread is also worth thing to do

Comment on attachment 464241 [details] [diff] [review] threadsafe ref counting for nsStrictTransportSecurityService Ah, I didn't see this patch before, all I ask you do here, perfect!

Comment on attachment 464041 [details] [diff] [review] interdiff: merge to m-c + new redirect API adaption Rewrite looks fine in general, but please clarify a few things before r+'ing... >+ if (mLoadGroup) >+ mLoadGroup->RemoveRequest(this, nsnull, mStatus); I don't see this in the original code... why? >+ if (NS_FAILED(rv)) { >+ // We have to manually notify the listener because there is not any >+ // pump that would call our OnStart/StopRequest after resume from >+ // waiting for the redirect callback. >+ DoNotifyListener(); >+ return rv; >+ } Clear mRedirectChannel? (Also missing if deprecated notification fails below.) >+ // open new channel >+ rv = mRedirectChannel->AsyncOpen(mListener, mListenerContext); >+ mRedirectChannel = nsnull; >+ if (NS_FAILED(rv)) { >+ mStatus = rv; >+ DoNotifyListener(); >+ return rv; >+ } >+ >+ Cancel(NS_BINDING_REDIRECTED); Original code just updates mStatus - why call Cancel() here?

(In reply to comment #83) > Comment on attachment 464041 [details] [diff] [review] > interdiff: merge to m-c + new redirect API adaption > > Rewrite looks fine in general, but please clarify a few things before r+'ing... > > >+ if (mLoadGroup) > >+ mLoadGroup->RemoveRequest(this, nsnull, mStatus); > > I don't see this in the original code... why? > The if (isStsHost) {} branch in nsHttpChannel::Connect previously returned NS_BINDING_ABORTED. AsyncOpen then called AsyncAbort that did the loadgroup remove. Now we proceed with NS_OK and cancel the channel later asynchronously in nsHttpChannel::ContinueAsyncRedirectChannelToHttps. > >+ if (NS_FAILED(rv)) { > >+ // We have to manually notify the listener because there is not any > >+ // pump that would call our OnStart/StopRequest after resume from > >+ // waiting for the redirect callback. > >+ DoNotifyListener(); > >+ return rv; > >+ } > > Clear mRedirectChannel? (Also missing if deprecated notification fails below.) > Good catch, will fix it. > >+ // open new channel > >+ rv = mRedirectChannel->AsyncOpen(mListener, mListenerContext); > >+ mRedirectChannel = nsnull; > >+ if (NS_FAILED(rv)) { > >+ mStatus = rv; > >+ DoNotifyListener(); > >+ return rv; > >+ } > >+ > >+ Cancel(NS_BINDING_REDIRECTED); > > Original code just updates mStatus - why call Cancel() here? Hmm.. also good catch, there is no reason to call Cancel() here.

(In reply to comment #57) > With respect to port numbers, I think that's not well covered in the spec. The > way it works in chrome is that the STS state is for the whole host (this is > important to protect secure cookies, which are shared by the whole host). For > the redirect behavior: port numbers are preserved except port 80 is changed to > port 443 because 80 is the default port for http. Hmm... how many servers are able to accept plain HTTP requests and also SSL/HTTP requests both on a single port?

I think most servers will generate an error. Sometimes they detect and will try to actually give a properly formatted error in HTTP, sometimes not. In either case I guess we'd expect a hard-fail for the client if the server isn't offering HTTPS on the given port. I view it as not that big a deal as the server should set the policy, and if they get it wrong, will have to cope with it. It is not a valid use case for HSTS that a host will offer both HTTP and HTTPS on different ports, and expect a client to ever successfully make an HTTP connection while HSTS is in effect. That behavior is precluded by the spec.

Comment on attachment 464229 [details] [diff] [review] HSTS Core Support (v6) >+nsHttpChannel::ProcessSTSHeader() >+ nsCOMPtr<nsIStrictTransportSecurityService> stss = gHttpHandler->GetSTSService(); >+ >+ // Check the trustworthiness of the channel (are there any cert errors?) >+ // If there are certificate errors, we still load the data, we just ignore >+ // any STS headers that are present. >+ NS_ENSURE_TRUE(mSecurityInfo, NS_ERROR_FAILURE); >+ PRBool tlsIsBroken = PR_FALSE; >+ rv = stss->ShouldIgnoreStsHeader(mSecurityInfo, &tlsIsBroken); >+ NS_ENSURE_SUCCESS(rv, rv); You don't verify stss is no-null here before using it. >@@ -1775,16 +1790,23 @@ nsHttpHandler::Observe(nsISupports *subj >+ else if (strcmp(topic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) { >+ nsCString switchType = NS_ConvertUTF16toUTF8(data); >+ if (strcmp(switchType.get(), NS_PRIVATE_BROWSING_ENTER) == 0) >+ mInPrivateBrowsingMode = PR_TRUE; >+ else if (strcmp(switchType.get(), NS_PRIVATE_BROWSING_LEAVE) == 0) >+ mInPrivateBrowsingMode = PR_FALSE; >+ } This is ok, just a nit: you can save coping the string by using 'NS_ConvertUTF16toUTF8 switchType(data);', up to you. >@@ -3397,42 +3398,72 @@ nsNSSBadCertHandler(void *arg, PRFileDes >+ // Enforce Strict-Transport-Security for hosts that are "STS" hosts: >+ // connections must be dropped when there are any certificate errors >+ // (STS Spec section 7.3). >+ // The StrictTransportSecurityService is not available because it's running >+ // in a different thread. Instead we must rely on the calling connection to >+ // set the STS state in the infoObject before we get here. I believe this comment is out-of-date. >+++ b/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_stricttransportsecurity.html Mon Aug 09 14:03:00 2010 -0700 >+ function loadVerifyFrames(round) { >+ for(var test in testframes) { Space after for. r=honzab with these few details fixed.

Comment on attachment 464241 [details] [diff] [review] threadsafe ref counting for nsStrictTransportSecurityService >+++ b/security/manager/boot/src/nsStrictTransportSecurityService.cpp Mon Aug 09 17:33:58 2010 -0700 >+NS_IMPL_THREADSAFE_ISUPPORTS1(nsStrictTransportSecurityService, nsIStrictTransportSecurityService) 80 chars per line please (wrap after the comma and indent nsIStrictTransportSecurityService as nsStrictTransportSecurityService. r=honzab.

(In reply to comment #86) > It is not a valid use case for HSTS that a host will offer both HTTP and HTTPS > on different ports, and expect a client to ever successfully make an HTTP > connection while HSTS is in effect. That behavior is precluded by the spec. I see. Also, when the spec allows port override, a potential attacker could temper with the header and redirect to its server, though limited by the same domain.

Update to v6, addressing Bjarne's comments. - mRedirectChannel = nsnull done automatically by AutoRedirectVetoNotifier - mStatus = NS_BINDING_REDIRECTED instead of call to Cancel

Comment on attachment 465420 [details] [diff] [review] redirect API adaption review comments update v1 Nice use of the Notifier.. :)

Comment on attachment 464229 [details] [diff] [review] HSTS Core Support (v6) >+ // if this is an Strict-Transport-Security host and the cert Nit: "an Strict-..." reads weirdly to me. Perhaps "if this is an STS host..." or "if this is a Strict-...". >diff -r 25a4b29f243b netwerk/protocol/http/nsHttpChannel.cpp >+ nsCOMPtr<nsIStrictTransportSecurityService> stss = gHttpHandler->GetSTSService(); Why does this have to be an nsCOMPtr? >+ NS_ASSERTION( ! (wasAlreadySTSHost && tlsIsBroken), >+ "connection should have been aborted by nss-bad-cert-handler"); Nit: why the crazy spaces around the !? >diff -r 25a4b29f243b netwerk/protocol/http/nsHttpHandler.cpp >+ else if (strcmp(topic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) { >+ nsCString switchType = NS_ConvertUTF16toUTF8(data); >+ if (strcmp(switchType.get(), NS_PRIVATE_BROWSING_ENTER) == 0) These inner strcmps read better as NS_LITERAL_STRING(NS_PRIVATE_BROWSING_{ENTER,LEAVE}).Equals(data) IMO. sr=mrbkap with those comments addressed.

Folded all the patches into one and merged to mozilla-central.

Comment on attachment 466484 [details] [diff] [review] HSTS Core Support (v7) This is large and touching places that changes often, would be good to land ASAP to prevent bitrotting.

(In reply to comment #94) > to prevent bitrotting. Already happend, its now bitrotted, due to checkin d0b284052d29 in nsHttpHandler.cpp. But its only a very light bitrotting.

FYI, d0b284052d29 was backed out, but we still need to land this ASAP as we are getting close to feature freeze for Firefox 4.

Was gonna get this, but patch failed to apply to tip... refresh? ;)

Fresh patch for bitrot (parent is rev 46f402c75824). Two very small hunks failed in the previous patch... this was a trivial refresh.

Comment on attachment 468708 [details] [diff] [review] HSTS Core Support (v7) [Check-in comment 99] http://hg.mozilla.org/mozilla-central/rev/5dc3c2d2dd4f

Is there a current specification for this?

Yeah... got buried in the comments: http://tools.ietf.org/html/draft-hodges-strict-transport-sec

Shawn pushed this changeset because this patch broke pymake: http://hg.mozilla.org/mozilla-central/rev/2f01056be931

Documented here: https://developer.mozilla.org/en/Security/HTTP_Strict_Transport_Security Linked to from: https://developer.mozilla.org/en/Security/Securing_your_site https://developer.mozilla.org/en/Firefox_4_for_developers#Security

It looks likely that this broke HTTPS connections through at least NTLM proxies. See bug 592197.

@sheppy: the current spec is here: https://tools.ietf.org/html/draft-ietf-websec-strict-transport-sec is there a way to change the "url" in the original bugzilla bug entry for Bug 495115 ?