Entries in the cert override databased are keyed off host:port strings, but the way these are handled is difficult to predict without looking at the code. For instance: http://mxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsCertOverrideService.cpp#486 When storing an override, the code checks for the presence of a ':' character, and if not present, appends ":443". However, the corresponding lookup code (hasMatchingOverride) doesn't "fix" hosts, instead assuming that the string passed in is in the right format. It's also unclear what would happen in the case of an IPv6 address, since the address octets are :-delimited. One possible solution would be to modify the interface to take host and port as separate arguments (possibly with port being optional?) and doing the necessary string-munging internally. Obviously this would involve updating the existing consumers. Discovered as part of gavin's review comments in bug 406612.

This bug has further implication, as I understand. Indexing exceptions with UTF-16 nsIURI.host (or hostPort) may lead to non-uniqueness of that index when working with IDNs. Therefor I suggest as part of the API fix to also move to using asciiHost portion of URI that is guarantied to return always the same result. Problem might become when representing the exception on a screen. It is more human readable in the UTF-8 or 16 form then in PUNICODE. However, displaying the host in punicode might help against IDN homograph attack. Other problem might raise with backward compatibility to lower FF3 version (not FF2) when reading the file cert_override.txt.

This patch fixes this original bug and also another bug with IDN security sites we need to add an exception for. I was testing w/o this patch (current trunk) using url http://mitsuketa-nihongo.jp/ that redirects to http://日本語.jp/case/accessible/. I changed url to https://日本語.jp/, added exception but it only shows again the same "page load error" message - exception could not be correctly added because PSM internaly works with punicoded URL. With this patch it works. URL is displayed in punicode form in the exception list (the server cert list) and also is stored that way in cert_override.txt. W/o this patch it is stored in UTF-8 encoding (what is wrong) and also this way displayed on the screen - as "日本語.jp:443" (what is user friendly but potentially dangerous). So, the only concern is how to display the domain in the server cert list. Probably display both punicode and UTF-8 form?

(In reply to comment #2) > way displayed on the screen - as "日本語.jp:443" (what > is user friendly but potentially dangerous). Is the homograph attack the only danger you are concerned about?

Are you looking for a fix for the Firefox 3 branch? Are we still allowed to change interfaces for Firefox 3?

I didn't think about IPv6 when I worked on the original code. If you want to fix the issue "colon used for both IPv6 and port separator", then you could change the implementation to be smart. Start searching from the end of the string for the port separator. We could change the definition of the API to always require a :port suffix, and fix the callers.

Personally I would say this should be made blocking for FF3. If fixes a problem that would be shame not to fix - adding exceptions for international domain names. Re homograph attack: I am not expert for security attacks and not any more for IDN in particular. However, it is IMO good to display also the punicoded name of the server in the exception list. I was not testing with IPv6 servers but I believe it fixes that too.

Comment on attachment 316196 [details] [diff] [review] Fix, v1 Please address at least the comments which I have marked with ### >- if (this._overrideService.hasMatchingOverride(lookupHost, iData.cert, {}, {})) >+ if (this._overrideService.hasMatchingOverride(this._lastLocation.hostname, this._lastLocation.port, iData.cert, {}, {})) optional nit: want to avoid very long line? > > overrideService.rememberValidityOverride( >- getURI().hostPort, >+ getURI().asciiHost, getURI().port, > gCert, > flags, > !permanentCheckbox.checked); Maybe I shouldn't worry about efficiency in JS, but I think it's more efficient to do var uri = getURI(); ... uri.asciiHost, uri.port ... >- * @param aHostNameWithPort The host:port this mapping belongs to >+ * @param aHostName The host this mapping belongs to >+ * @param aPort The port this mapping belongs to ### The comments for the hostname should make it clear we're expecting ACE / punicode (not UTF-8). >+ nsCAutoString host(tmp); I think want to use some lossy conversion instead of copying, maybe NS_LossyConvertUTF16toASCII host(tmp) ? >- AddEntryToList(host, >+ PRInt32 port; >+ PRInt32 portIndex = host.RFindChar(':'); >+ if (portIndex == kNotFound) >+ port = 443; // default port number >+ else { >+ nsresult portParseError; Would prefer "rvParse", but that's really a nit (it's a result, we don't know whether it's an error). Feel free to ignore. >+ nsCAutoString portString(Substring(host, portIndex+1)); >+ port = portString.ToInteger((PRInt32*)&portParseError); Is it really necessary to copy the string? In other words, does this work Substring(...).ToInteger(...) ? If that doesn't work, could you use nsDependentCString to avoid copying? >+ if (NS_FAILED(portParseError)) >+ port = 443; // What to do... we have to assign the default value I'd even reject this invalid file, if what we get here is not a number, but ok. >+ else >+ host.Truncate(portIndex); >+ } >+ >+ AddEntryToList(host, port, > PR_FALSE, // not temporary > algo_string, fingerprint, bits, db_key); >-nsCertOverrideService::RememberValidityOverride(const nsAString & aHostNameWithPort, >+nsCertOverrideService::RememberValidityOverride(const nsACString & aHostName, PRInt32 aPort, > nsIX509Cert *aCert, > PRUint32 aOverrideBits, > PRBool aTemporary) > { > NS_ENSURE_ARG_POINTER(aCert); >- if (aHostNameWithPort.IsEmpty()) >+ if (aHostName.IsEmpty()) > return NS_ERROR_INVALID_ARG; ### We should have sanity checking for allowed port numbers, at least when adding to storage. Let's at least reject negative ports. >+void >+nsCertOverrideService::GetHostWithPort(const nsACString & aHostName, PRInt32 aPort, nsACString& _retval) >+{ >+ nsCAutoString hostPort(aHostName); >+ if (aPort == -1) >+ aPort = 443; ### I don't understand why you allow -1 as a special input. You don't define the meaning of this anywhere, not in the IDL. I think we should not accept negative port numbers and our internal default can be 443. > nsCertOverride() > :mOverrideBits(ob_None) >+ ,mPort(-1) ### You could use 443 as the default value, then you don't have to check for -1. >+ // Concates host name and the port number. If the port number is -1 then >+ // port 443 is automatically used. This method ensures there is always a port >+ // number separated with colon. >+ static void GetHostWithPort(const nsACString & aHostName, PRInt32 aPort, nsACString& _retval); Ok, here you explain the special meaning of -1, but you never define it elsewhere in the IDL, for the IDL functions that call this helper funciton. I think we should not do special handling for -1 here. > nsCertTreeDispInfo::nsCertTreeDispInfo() > :mAddonInfo(nsnull) > ,mTypeOfEntry(direct_db) > ,mOverrideBits(nsCertOverride::ob_None) > ,mIsTemporary(PR_TRUE) >+,mPort(-1) I guess here it is ok to use -1. > nsCertTreeDispInfo *certdi = new nsCertTreeDispInfo; I will make a single comment for all your changes related to nsCertTreeDispInfo. IMHO the UI in cert manager should continue to display the human readable IDN. I don't see an attack vector here, this display is only used after the exception got added. But when managing the list of stored exceptions, IMHO it's easier to find entries by looking for the IDN. I guess you could omit most of your changes to "certdi", and use nsIIDNService to get the display string? But your code appears to be correct, so we could fix that later.

Honza, FYI: Yesterday in our chat, we had discussed to keep the interface as much as possible, and instead fix the implementation. But then we had another discussion on IRC on #developers with Shaver, Gavin and Johnath and Smontagu. I have learned there are multiple UTF 16 representations for the same hostname, and that is indeed a good argument against using UTF 16, and switch to the ascii encoding for storage and interface. Based on the arguments, Shaver has agreed to allow the interface change, assuming we change it immediately. Shaver has also argued, if we change it, we must change it BEFORE the FF 3 release, and there I request this bug should block.

I've changed the subject of the bug. We are really trying to fix the issue that we can't deal with IDN, and in order to avoid ambiguities and make it work with the networking code, we must switch to the ascii encoding of domain names.

(In reply to comment #7) > >- if (this._overrideService.hasMatchingOverride(lookupHost, iData.cert, {}, {})) > >+ if (this._overrideService.hasMatchingOverride(this._lastLocation.hostname, this._lastLocation.port, iData.cert, {}, {})) > > optional nit: want to avoid very long line? done > > overrideService.rememberValidityOverride( > >- getURI().hostPort, > >+ getURI().asciiHost, getURI().port, > > gCert, > > flags, > > !permanentCheckbox.checked); > > Maybe I shouldn't worry about efficiency in JS, but I think it's more efficient > to do > var uri = getURI(); > ... uri.asciiHost, uri.port ... done > >- * @param aHostNameWithPort The host:port this mapping belongs to > >+ * @param aHostName The host this mapping belongs to > >+ * @param aPort The port this mapping belongs to > > ### > The comments for the hostname should make it clear we're expecting > ACE / punicode (not UTF-8). done > >+ nsCAutoString host(tmp); > > I think want to use some lossy conversion instead of copying, maybe > NS_LossyConvertUTF16toASCII host(tmp) > ? didn't work, kept unchanged. It's already a CString, you did this because you modify the code further down, so it's fine. > >- AddEntryToList(host, > >+ PRInt32 port; > >+ PRInt32 portIndex = host.RFindChar(':'); > >+ if (portIndex == kNotFound) > >+ port = 443; // default port number > >+ else { > >+ nsresult portParseError; > > Would prefer "rvParse", but that's really a nit (it's a result, we > don't know whether it's an error). Feel free to ignore. I've learned, it's not an nsresult, but a PRInt32. I've changed the type and kept the name, and changed the error checking (copied from other code in content, using NS_FAILED seems incorrect, because it's not a nsresult). > >+ nsCAutoString portString(Substring(host, portIndex+1)); > >+ port = portString.ToInteger((PRInt32*)&portParseError); > > Is it really necessary to copy the string? > In other words, does this work > Substring(...).ToInteger(...) > ? > > If that doesn't work, could you use nsDependentCString to avoid copying? Both my proposals didn't work, kept unchanged. > >+ if (aHostName.IsEmpty()) > > return NS_ERROR_INVALID_ARG; > > ### > We should have sanity checking for allowed port numbers, at least when adding > to storage. > Let's at least reject negative ports. Added. > >+nsCertOverrideService::GetHostWithPort(const nsACString & aHostName, PRInt32 aPort, nsACString& _retval) > >+{ > >+ if (aPort == -1) > >+ aPort = 443; > > ### > I don't understand why you allow -1 as a special input. > You don't define the meaning of this anywhere, not in the IDL. Check removed, default changed to 443. > > nsCertOverride() > > :mOverrideBits(ob_None) > >+ ,mPort(-1) > > ### > You could use 443 as the default value, then you don't have to check for -1. changed > >+ // Concates host name and the port number. If the port number is -1 then > >+ // port 443 is automatically used. This method ensures there is always a port > >+ // number separated with colon. Comment updated. > I will make a single comment for all your changes related to > nsCertTreeDispInfo. > IMHO the UI in cert manager should continue to display the human readable IDN. For now I've kept your code. We can tweak this later.

I'm ok with your patch with these changes.

FYI, your patch had produced compiler warnings, because of incorrect init order in the constructor, that's why I changed the position of mPort init.

Hmm, this patch doesn't work for me. I wonder if I broken Honzas patch in my attempts to improve it? When I attempt to add an exception I get Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICertOverrideService.rememberValidityOverride]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://pippki/content/exceptionDialog.js :: addException :: line 350" data: no] Source File: chrome://pippki/content/exceptionDialog.js Line: 350

(In reply to comment #13) > Hmm, this patch doesn't work for me. > I wonder if I broken Honzas patch in my attempts to improve it? > > When I attempt to add an exception I get > > Error: [Exception... "Component returned failure code: 0x80070057 > (NS_ERROR_ILLEGAL_VALUE) [nsICertOverrideService.rememberValidityOverride]" > nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: > chrome://pippki/content/exceptionDialog.js :: addException :: line 350" data: > no] > Source File: chrome://pippki/content/exceptionDialog.js > Line: 350 > That's exactly the reason why I was playing with -1 values for the port in my version of the patch. nsIURI might return -1 as the port number what doesn't mean invalid port but default port number. If you want to filter -1 as input (INVALID_ARG) then you have to convert -1 in JS to default port number (using some code like: port = uri.port = -1 ? Cc[mozilla.org/ioservice;1"].getService(Ci.nsIOService).getDefaultPort(uri) : uri.port;) or do this in nsCertOverrideService in C++ but then you have to change the interface to take nsIURI. This is reason I decided to allow -1 as port number and in that case default it to 443. Question is what what to do in case of snews:// URI because it uses port 536 (or some around as I remember).

This includes all Kaie's changes and still allows -1 as port argument and defaults it to 443. Works also for adding manual exceptions. My note about 'snews' is about using of nntp protocol over TLS/SSL, port 563.

Comment on attachment 316983 [details] [diff] [review] v3 Ok, thanks for the explanation. I still think it's not a good idea to allow -1 as an input port number. Although -1 means "default port of the protocol" in nsIURI, we have no idea what protocol we deal with in the exception service. I would really prefer to be strict and require that the caller specifies a correct port number. But given that our implementation already allowed a missing port to mean port 443, I'm ok to define that port -1 means 443. However, that must not be undocumented. If you decide to allow -1, we must document it in the IDL (in all places where -1 is allowed for the port number). Please remove the code the allows 443 when reading from the config file. I think a missing port is an error. I think our existing implementation always stores a port number. If you find an entry without port number, we should skip (ignore) that line.

Ok, I added comments to IDL file on all places. I agree it is not absolutely correct this way but then we should probably pass nsIURI object where possible, at least when creating new exception.

Comment on attachment 317001 [details] [diff] [review] v4 [Checkin: Comment 23] r=kengert, thanks Honza! Requesting approval for FF 3 for this patch with interface changes.

Comment on attachment 317001 [details] [diff] [review] v4 [Checkin: Comment 23] I don't see any tests in this patch, and a quick scan of the tree indicates that there aren't any existing tests for the override service. We're pressed for time, so I'd be willing to approve if there were a test plan sketched in the bug, that can be implemented (immediately) after checkin. Please re-request approval with such a test plan in the bug! Also, it looks like GetCellText might want to check the return from GetHostWithPort, no?

Test plan: - go to IDN enabled secure server with incorrect certificate for which you didn't ever entered any security exception, e.g. https://日本語.jp/ - add exception for that page as usual expected: page content load unexpected: page load error appears again as at the start

To automate the test: 1. have a testing secure http server listening on port 443 2. let the server be accessible through international domain name 3. let the server's certificate be one of these: - validity expired - cert domain mismatched from the international domain name address - cert is selfsigned or signed by untrusted issuer 4. let there be no security exception for that server cert in the testing profile 5. open (e.g. in iframe) a test secure page hosted on the server using the intl domain name to access it 6. check the page failed to load because of a security issue with the certificate 7. call nsICertOverrideService.rememberValidityOverride with punicoded domain name address of the server and -1 as a port number 8. reload the test page again 9. check the page content loaded correctly 10. call nsICertOverrideService.hasMatchingOverride for ascii host representation of the iframe location (nsIURI.asciiHost) and check the result is: true for aPort = -1 true for aPort = 443 false for aPort is not -1 nor 443 11. call nsICertOverrideService.hasMatchingOverride for host name of the iframe location only converted to UTF8 (nsIURI.host) and check the result is false for port = -1, 443, a number different from -1 and 443

Comment on attachment 317001 [details] [diff] [review] v4 [Checkin: Comment 23] a=shaver, get crackin' on those tests. :)

mozilla/browser/base/content/browser.js 1.1029 mozilla/security/manager/pki/resources/content/exceptionDialog.js 1.10 mozilla/security/manager/ssl/public/nsICertOverrideService.idl 1.3 mozilla/security/manager/ssl/src/nsCertOverrideService.cpp 1.6 mozilla/security/manager/ssl/src/nsCertOverrideService.h 1.4 mozilla/security/manager/ssl/src/nsCertTree.cpp 1.65 mozilla/security/manager/ssl/src/nsCertTree.h 1.20 mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp 1.160

This is preview of mochitest for this bug. It is dependent on patches from bug 428009. It is chrome test because of need to easily add an exception and hook bad cert callback. I try to load a host with self signed certificate within a frame. I expect it fails, then I add an exception (the same way the add exception dialog does) and reload the frame expecting it loads. I also move xpc-shell tests to a separate 'unit' dir, it seems appropriate to me. I will create hg version of the patch, because there is a new binary file etc.

This is final version according to comment 21.

Comment on attachment 322645 [details] [diff] [review] Test, v1 If you've tested this, r=kaie

Adding a dependency to bug 428009, it's required for the test we want to add here.

Honza, is the test now ready to land in mozilla-central?

(In reply to comment #28) > Honza, is the test now ready to land in mozilla-central? It is not. Something has changed in the inter-window property access rights and main window is no more communicating with the iframe. I want to make it work again today and then we can land it.

Working test, v2. The same functionality but slightly different system. Kai, please just take a quick look.

Comment on attachment 337863 [details] [diff] [review] Test, v2 [Checkin: Comment 31] http://hg.mozilla.org/mozilla-central/rev/097bf47abbcd

http://hg.mozilla.org/mozilla-central/rev/949bf9720ad2 { Disable the test for Bug 413909 - nsCertOverrideService IDN handling is broken; tests; r=kaie which seems to leak the world. } See for example http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1223650113.1223653649.16735.gz Reopening, as I don't know which one of the fix or the test to blame.

http://hg.mozilla.org/mozilla-central/rev/aea2d7b68a3d { Fix build bustage after workaround for Bug 413909 - nsCertOverrideService IDN handling is broken; tests; r=kaie } I guess an empty list is not supported :-<

(In reply to comment #33) http://hg.mozilla.org/mozilla-central/rev/56d8e1c0d95e Well, the command does not like "no/empty argument" rather :-<

I can reproduce it. Will take a look why we leak so much. Leaks are always different (random).

Problem is missing Release call or a cycle on class implementing nsIBadCertListener2. The same leak is reproduced when I add an exception for a certificate and exit (exactly: open the exception dialog, click 'view certificate', close dialog, close FF). To remove the leak I do the following: req.open("GET", "https://bug413909.xn--hxajbheg2az3al.xn--jxalpdlp/", false); req.channel.notificationCallbacks = certListener; + certListener.QueryInterface(Components.interfaces.nsISupports).Release(); req.send(null); Leak also disappers when I comment 'notificationCallbacks = certListener' assignment. I will investigate further and create a new bug when I find something more specific.

Based on this description, it seems likely to me that this is the root of bug 426076 as well. Marking this bug as blocking that one, since I'm not confident they are duplicates yet (MitM Me has its own codepath, but it is copied from the code in the Add Exception dialog).

(In reply to comment #37) > Based on this description, it seems likely to me that this is the root of bug > 426076 as well. Marking this bug as blocking that one, since I'm not confident > they are duplicates yet (MitM Me has its own codepath, but it is copied from > the code in the Add Exception dialog). I rechecked the leak log with patch for bug 459657 and there are 4 nsStringBuffers leaking, always; previously overlooked.

(In reply to comment #38) > (In reply to comment #37) > > Based on this description, it seems likely to me that this is the root of bug > > 426076 as well. Marking this bug as blocking that one, since I'm not confident > > they are duplicates yet (MitM Me has its own codepath, but it is copied from > > the code in the Add Exception dialog). > > I rechecked the leak log with patch for bug 459657 and there are 4 > nsStringBuffers leaking, always; previously overlooked. The patch in bug 475702 should fix the stringbuffer leaks (and the badcertlistener leaks too but I guess I'll strip that out as it is covered in bug 459657)

OK, I made this bug dep on bug 475702 as well then.

http://hg.mozilla.org/mozilla-central/rev/fc19648c0f72 Just re-enabled the test, not fix to this bug itself.