From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.9+) Gecko/20020308 BuildID: 2002030803 When a URL is entered in the form ftp://username:password@sitename the complete string is saved into the history, including the password. Username and password is readable to anyone, which means that this is a security issue. Reproducible: Always Steps to Reproduce: 1.Go to any ftp site by typing in ftp://username:password@sitename 2. 3. Actual Results: The UserID and password are saved in History. Expected Results: The ftp site only should be saved in the History and not the userid and password

It appears that 4.x did remove the password (not the userid) when placing it in the history. Note it is still in the URL bar dropdown.

oy. This should probably be done in docshell, where the page gets added docshell/base/nsDocShell.cpp.. though I kind of think that if someone puts a password in their url, then they deserve to lose :) what do 4.x and IE do?

4.x removes it, IE does not. > though I kind of think that if someone puts a > password in their url, then they deserve to lose :) I thought that as well, but this customer is kind of adamant about it.

*** Bug 131318 has been marked as a duplicate of this bug. ***

This is a possible fix. Query the password before adding to history and if it exists, set it to blank, then set it back when we are done. This keeps us from having to do ugly string manipulation on the URI. The other thing I thought of was a "GetSpecWithoutPassword" API on URI, but that seems like overkill.

Comment on attachment 75274 [details] [diff] [review] Possible fix I'm personally reluctant to do this - I mean, lets say I have a url ftp://foo@bar.com/ and then I enter my password in the UI. will my password get inserted into history as ftp://foo:passwd@bar.com/? if not, then this seems like much less of an issue.. hrm. I just don't know. But, say I were to review this anyway, here's what I would say: :) instead of "blankpw" you can just say NS_LITERAL_CSTRING("") - no need for nsCAutoString here... and "pw" should be "password" - there's no need to shorten this variable name. Variable names are cheap :)

I think the main reason to do this is because it worked this way in 4.x. Do you think the removing and adding the password is OK or is it too kludgy?

reassigning.

This is a major security issue, so please assign target milestone. The fact that MSIE doe the same isn't an issue, or is it?

CC'ing and s/doe/does

Narrowing summary.

*** Bug 146289 has been marked as a duplicate of this bug. ***

*** Bug 146289 has been marked as a duplicate of this bug. ***

The patch seems work well. Hope it can be checked in soon with a little modification. By the way, IE works the way just as the mozilla without the patch. IE remembers passwd in its history too. By the way, I still think it is the user's fault to type in the passwd into URL field with user name.

With Michael Kaply's patch, there's still a problem. When the connection to the website is fail, it generates an Error item in the History, and the user name and password are still there.

Well, if anyone wants to add this to their build, here is an updated patch.

Bug 186695 is related to this bug. Can the patch be updated to include "print" and "print preview" as well?

Comment on attachment 98947 [details] [diff] [review] updated patch You have a better chance of being reviewed if you actually ask for it.

Comment on attachment 98947 [details] [diff] [review] updated patch + uriToAdd.password = ""; + urlToAdd = uriToAdd.spec; is this really what you intend? it looks like you're converting a url object to a string... + // Now get the new url string (without the password) + uri->GetSpec(url); + newURLString = url.get(); woah, no! never save the char value from a .get()! there's no guarantee about how long that pointer will be valid. I'm also not psyched about creating a new url object each time a url is added to global history.. I think at the very least that we've already created such an object in nsDocShell - we should put the fix there..

*** Bug 225309 has been marked as a duplicate of this bug. ***

Michael: What would it take to make a patch that fits your suggestions? Also, before we checkin, I'd like to make sure that password manager records passwords correctly in these situations, that would let the user session still work smoothly as far as password entry, but not store passwords in plain text throught the profile.

I left bug 250337 open since I couldn't find a Firefox bug for this. Not sure if a patch for this will fix Firefox.

Mozilla should not under any circumstances save any passwords!! This is a serious security hole - especially in public places like universities or schools. Anyone sitting at a computer after someone else sees his or her password when logging in to gmx etc. Mozilla has a reputation as a very secure browser - much better than msexplorer. This password saving defeats the purpose!!! Yours David Paenson Frankfurt University of Applied Sciences (Germany)

(In reply to comment #0) I logged in to file this bug, looks like it's been sitting around for quite a while. Microsoft fixed it in IE6. ;)

I'm adding this to my list of likely network bugs to work on. No guarantee when I'll actually get to it.

Hope I'm not stepping on anyone's toes here. I've implemented Alec's suggestion of sanitizing inside nsDocShell and tested against my local server with authentication.

Comment on attachment 432407 [details] [diff] [review] Sanitize input URI when adding to session/global history r- on this only because it doesn't apply any more, things changed around a bit in nsDocShell::AddToGlobalHistory(). Josh, can you provide an updated patch? I can review...

Unbitrotted.

Comment on attachment 444241 [details] [diff] [review] Sanitize input URI when adding to session/global history - In nsDocShell::AddToSessionHistory(): - entry->Create(aURI, // uri + entry->Create(uri, // uri EmptyString(), // Title inputStream, // Post data stream Maybe add a space on the changed line to maintain the consistent comment indentation? r=jst

Nit addressed.

making a test for this should not be too hard I guess?

This patch is not ready to land since it is missing tests.

Fixed bitrot from jdm's patch

Tests to make sure username/password are not stored in global or session history.

Comment on attachment 562229 [details] [diff] [review] Places tests Review of attachment 562229 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/browser/browser_bug130327.js @@ +13,5 @@ > + * > + * The Original Code is mozilla.org code. > + * > + * The Initial Developer of the Original Code is > + * Mozilla Corporation. the Mozilla Foundation. (if you wish in tests you can use the shorter pd license boilerplate) @@ +38,5 @@ > +/** > + * Test for Bug 130327. Make sure that passwords typed in URLs are not saved in history > + **/ > + > +var testNum = 1; nit: test is using half let half var, we usually prefer to avoid mixing up unless willing to be explicit, just s/var/let/ or be consistent @@ +53,5 @@ > + const dialogDelay = 10; > + > + // Use a timer to invoke a callback to twiddle the authentication dialog > + timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > + timer.init(observer, dialogDelay, Ci.nsITimer.TYPE_ONE_SHOT); Is there any event we may listen for the auth dialog rather than guessing a timeout? Or a windows listener? see browser_bug511456.js for example, it seems more reliable. @@ +63,5 @@ > + Ci.nsISupports, Ci.nsISupportsWeakReference]; > + > + if (!interfaces.some( function(v) { return iid.equals(v) } )) > + throw Components.results.NS_ERROR_NO_INTERFACE; > + return this; Use XPCOMUtils.generateQI here please... Well, actually nsITimer.init() may just get a function passed in and there is no need to have an observer object at all @@ +68,5 @@ > + }, > + > + observe : function (subject, topic, data) { > + netscape.security.PrivilegeManager > + .enablePrivilege('UniversalXPConnect'); I think this is not needed in a b-c test @@ +84,5 @@ > + // through all the open windows and all the <browsers> in each. > + var wm = Cc["@mozilla.org/appshell/window-mediator;1"]. > + getService(Ci.nsIWindowMediator); > + //var enumerator = wm.getEnumerator("navigator:browser"); > + var enumerator = wm.getXULWindowEnumerator(null); Use Services.wm. the commented out code seems leftover? @@ +101,5 @@ > + if (childDocShell.busyFlags != Ci.nsIDocShell.BUSY_FLAGS_NONE) > + continue; > + var childDoc = childDocShell.QueryInterface(Ci.nsIDocShell) > + .contentViewer > + .DOMDocument; nit: align dots @@ +103,5 @@ > + var childDoc = childDocShell.QueryInterface(Ci.nsIDocShell) > + .contentViewer > + .DOMDocument; > + > + //ok(true, "Got window: " + childDoc.location.href); either delete or uncomment (converting to a proper info()) @@ +122,5 @@ > + * timer is a one-shot). > + */ > +function handleDialog(doc, testNum) { > + ok(true, "handleDialog running for test " + testNum); > + ok(true, "Time is " + (new Date()).toUTCString()); plse use info(msg) instead of ok(true, msg) @@ +127,5 @@ > + > + var dialog = doc.getElementById("commonDialog"); > + dialog.acceptDialog(); > + > + ok(true, "handleDialog done"); ditto @@ +135,5 @@ > +function test() { > + waitForExplicitFinish(); > + > + startCallbackTimer(); > + let tab = gBrowser.addTab('http://user:password@example.com'); let tab = gBrowser.selectedTab = gBrowser.addTab @@ +140,5 @@ > + > + let observer = { > + onTitleChanged: function() {}, > + onBeginUpdateBatch: function() { }, > + onEndUpdateBatch: function() { }, nit: move these with other empty methods after onVisit @@ +143,5 @@ > + onBeginUpdateBatch: function() { }, > + onEndUpdateBatch: function() { }, > + onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID, > + aTransitionType) { > + is(aURI.spec, 'http://example.com/', 'history URI should not contain the password'); I'd suggesto to add a TEST_URL and TEST_EXPOSABLE_URL consts to the top of the test and use those @@ +152,5 @@ > + > + is(shEntry.URI.spec, 'http://example.com/', 'session history URI should not contain the password'); > + > + > + gBrowser.removeCurrentTab(); lots of newlines in this method... @@ +153,5 @@ > + is(shEntry.URI.spec, 'http://example.com/', 'session history URI should not contain the password'); > + > + > + gBrowser.removeCurrentTab(); > + waitForClearHistory(finish); is this guaranteed to run after the auth dialog has been dismissed? looks like recipe for random failures, maybe there should be a maybeFinish method that waits both the history entry has been added and the auth dialog dismissed.

Comment on attachment 562229 [details] [diff] [review] Places tests waiting next iteration, thanks for looking at the test!

The dialog test code was modified from toolkit/components/places/tests/mochitest/prompt_common.js and I didn't realize it wasn't up to current standards. I've switched to using dialog code from browser_bug511456.js as suggested. All of the other review comments are also addressed.

Comment on attachment 562657 [details] [diff] [review] Places tests v.2 Review of attachment 562657 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Matthew N. [:MattN] from comment #41) > The dialog test code was modified from > toolkit/components/places/tests/mochitest/prompt_common.js and I didn't > realize it wasn't up to current standards. yeah sorry, some tests are really old but rewriting them is time consuming Looks much better, something may be improved but it looks good. ::: toolkit/components/places/tests/browser/browser_bug130327.js @@ +37,5 @@ > + }, > + > + onOpenWindow: function(win) { > + let domwindow = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor) > + .getInterface(Components.interfaces.nsIDOMWindow); nit: You can use Ci @@ +41,5 @@ > + .getInterface(Components.interfaces.nsIDOMWindow); > + > + let self = this; > + domwindow.addEventListener("load", function() { > + domwindow.removeEventListener("load", arguments.callee, false); arguments.callee is deprecated in ES5 strict mode, so rather give a label to the function and use the function name here @@ +43,5 @@ > + let self = this; > + domwindow.addEventListener("load", function() { > + domwindow.removeEventListener("load", arguments.callee, false); > + self.windowLoad(domwindow); > + }, false); I think it's safe to use capture = true here since regardless we executeSoon later and it's harder something will cancel it (remember to also fix removeEventListener) @@ +50,5 @@ > + onCloseWindow: function(win) { > + }, > + > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIWindowMediatorListener, > + Components.interfaces.nsISupports]) nit: you can use Ci and generateQI adds nsISupports automatically, so no need to provide it in the list @@ +73,5 @@ > + let index = gBrowser.sessionHistory.index; > + let shEntry = gBrowser.sessionHistory.getEntryAtIndex(index, false); > + > + is(shEntry.URI.spec, TEST_EXPOSABLE_URL, 'session history URI should not contain the password'); > + cleanupTest(); you don't need to put cleanupTest both here and in registerCleanupFunction, also because registerCleanupFunction can only do synchronous stuff (no waitForClearHistory(finish);) I suggest here you just invoke waitForClearHistory(finish); At the end of test() you just directly do: registerCleanupFunction(function () { gBrowser.removeCurrentTab(); Services.wm.removeListener(WindowWatcher); if (!PlacesObserver.lastVisited) PlacesUtils.history.removeObserver(PlacesObserver, false); }); And this should be enough, no need to track cleaning. @@ +80,5 @@ > +let PlacesObserver = { > + lastVisited: null, > + onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID, > + aTransitionType) { > + this.lastVisited = aURI; nit: since you only care about the spec, you may store this.lastVisitedURL = aURI.spec @@ +81,5 @@ > + lastVisited: null, > + onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID, > + aTransitionType) { > + this.lastVisited = aURI; > + maybeCheckHistory(); add PlacesUtils.history.removeObserver(this, false); before maybeCheckHistory() since at this point we are done and should stop listening.

Comment on attachment 562228 [details] [diff] [review] Sanitize input URI when adding to session/global history (un-bitrot) + nsCOMPtr<nsIURI> uri = aURI; + if (sURIFixup) { + nsCOMPtr<nsIURI> exposableURI; + rv = sURIFixup->CreateExposableURI(uri, getter_AddRefs(exposableURI)); No need for exposableURI here, simply pass aURI as the input argument here, and use uri as the out param, save yourself some refcounting overhead. Same patter occurs in the second hunk in this patch too, and the same comment applies there too. r=jst, but given what code this is in, I'd like another pair of eyes on this change.

Comment on attachment 562228 [details] [diff] [review] Sanitize input URI when adding to session/global history (un-bitrot) Use -P when creating patches But in general, I don't understand the first part. Why do we want to strip password from *session* history.

Has any thought been given to putting the createExposableURI call in nsNavHistory::AddURI itself, rather than at this one specific call site? Are there valid use cases for adding URIs with passwords to global history?

I didn't realize I wasn't CC'd. Also, the try run[1] uncovered two test failures which I'm investigating: 75094 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug172261.html | Subframe should be able to call us - got false, expected true 75149 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug255820.html | Wrong color after reload - got rgb(0, 0, 0), expected rgb(0, 180, 0) (In reply to Olli Pettay [:smaug] from comment #44) > But in general, I don't understand the first part. Why do we want to strip > password from *session* history. So that the username/password is not exposed through back/forward navigation, if I understand correctly. (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #45) > Has any thought been given to putting the createExposableURI call in > nsNavHistory::AddURI itself, rather than at this one specific call site? Are > there valid use cases for adding URIs with passwords to global history? I think this makes sense if jst/smaug agree. [1] https://tbpl.mozilla.org/?tree=Try&rev=6d3dc1179c8a

AddURI is not the only entry point for URIs, it's the entry point for synchronous APIs. There are other 2 asynchronous entry points in History.cpp and AsyncFaviconHelpers.cpp that should be fixed. As a side question, may a user want to add a bookmark with credentials in it? Should we just consider that a bad habit?

Nevermind the bookmarks part, we use AddURIInternal, not AddURI there. Still which operations adding credentials to the database should be allowed?

> (In reply to Olli Pettay [:smaug] from comment #44) > > But in general, I don't understand the first part. Why do we want to strip > > password from *session* history. > > So that the username/password is not exposed through back/forward > navigation, if I understand correctly. And do we want that? Are we sure that doesn't actually break some pages? What do other browsers do? Is that kind of behavior specified in HTML spec? > > (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #45) > > Has any thought been given to putting the createExposableURI call in > > nsNavHistory::AddURI itself, rather than at this one specific call site? Are > > there valid use cases for adding URIs with passwords to global history? > > I think this makes sense if jst/smaug agree. Sounds good to me.

MattN, feel free to keep running with this. It's off my radar for the moment.

(In reply to Olli Pettay [:smaug] from comment #49) > > (In reply to Olli Pettay [:smaug] from comment #44) > > > But in general, I don't understand the first part. Why do we want to strip > > > password from *session* history. > > > > So that the username/password is not exposed through back/forward > > navigation, if I understand correctly. > > And do we want that? Are we sure that doesn't actually break some pages? > What do other browsers do? Is that kind of behavior specified in HTML spec? Once you're logged in, that's cached in netwerk, so it shouldn't really matter for history navigation purposes. Given the relative rarity of these kinds of URLs, I'd suspect any potential breakage is deep in edgecase-land, and could just be a WONTFIX. Unless there's some obvious case we know this will break, I think we should try this as-is.