Passwords in urls are saved in history

NEW
Unassigned

Status

()

Toolkit
Places
P3
major
16 years ago
4 months ago

People

(Reporter: Calvin, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {sec-low})

Trunk
sec-low
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low local])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

16 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

16 years ago
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.
Severity: normal → major
Keywords: 4xp
OS: Windows NT → All
Hardware: PC → All

Comment 2

16 years ago
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?

Comment 3

16 years ago
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. ***

Comment 5

16 years ago
Created attachment 75274 [details] [diff] [review]
Possible fix

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 6

16 years ago
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 :)
Attachment #75274 - Flags: needs-work+

Comment 7

16 years ago
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?

Comment 8

16 years ago
reassigning.
Assignee: blaker → mkaply

Comment 9

16 years ago
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?

Comment 10

16 years ago
CC'ing and s/doe/does

Comment 11

16 years ago
Narrowing summary.
Summary: Security issue because then anyone can access site using History → Passwords in urls are saved in history

Comment 12

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

Comment 14

16 years ago
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.

Comment 15

16 years ago
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.
Created attachment 98947 [details] [diff] [review]
updated patch

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

Updated

15 years ago
Attachment #75274 - Attachment is obsolete: true

Comment 17

15 years ago
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.
Attachment #98947 - Flags: review?(alecf)

Comment 19

15 years ago
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..

Updated

15 years ago
Attachment #98947 - Flags: review?(alecf) → review-

Comment 20

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

Comment 21

14 years ago
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.
QA Contact: claudius → benc

Updated

14 years ago
Blocks: 146289

Updated

14 years ago
Blocks: 233340
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.

Comment 23

13 years ago
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)
Blocks: 312321
Depends on: 313856

Comment 24

12 years ago
(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. ;)
Assignee: mozilla → nobody
QA Contact: benc → history.global

Updated

10 years ago
Duplicate of this bug: 404361

Updated

10 years ago
Whiteboard: [sg:low local]
I'm adding this to my list of likely network bugs to work on.  No guarantee when I'll actually get to it.
Assignee: nobody → jduell
Duplicate of this bug: 456955

Updated

9 years ago
Duplicate of this bug: 488573
Created attachment 432407 [details] [diff] [review]
Sanitize input URI when adding to session/global history

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.
Attachment #432407 - Flags: review?(bzbarsky)
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...
Attachment #432407 - Flags: review?(bzbarsky) → review-
Created attachment 444241 [details] [diff] [review]
Sanitize input URI when adding to session/global history

Unbitrotted.
Assignee: jduell.mcbugs → josh
Attachment #98947 - Attachment is obsolete: true
Attachment #432407 - Attachment is obsolete: true
Attachment #444241 - Flags: review?(jst)
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
Attachment #444241 - Flags: review?(jst) → review+
Created attachment 445283 [details] [diff] [review]
Patch

Nit addressed.
Attachment #444241 - Attachment is obsolete: true

Updated

8 years ago
Keywords: checkin-needed
making a test for this should not be too hard I guess?
Flags: in-testsuite?
This patch is not ready to land since it is missing tests.
Keywords: checkin-needed
Blocks: 602181
Created attachment 562228 [details] [diff] [review]
Sanitize input URI when adding to session/global history (un-bitrot)

Fixed bitrot from jdm's patch
Attachment #445283 - Attachment is obsolete: true
Attachment #562228 - Flags: review?(jst)
Created attachment 562229 [details] [diff] [review]
Places tests

Tests to make sure username/password are not stored in global or session history.
Attachment #562229 - Flags: review?(mak77)
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!
Attachment #562229 - Flags: review?(mak77)
Created attachment 562657 [details] [diff] [review]
Places tests v.2

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.
Attachment #562229 - Attachment is obsolete: true
Attachment #562657 - Flags: review?(mak77)
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.
Attachment #562657 - Flags: review?(mak77) → review+
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.
Attachment #562228 - Flags: superreview?(Olli.Pettay)
Attachment #562228 - Flags: review?(jst)
Attachment #562228 - Flags: review+
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.

Updated

6 years ago
Attachment #562228 - Flags: superreview?(Olli.Pettay) → superreview-
MattN, feel free to keep running with this. It's off my radar for the moment.
Assignee: josh → nobody
Keywords: sec-low
(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.

Updated

2 years ago
Flags: in-testsuite?

Updated

6 months ago
Component: History: Global → Places
Product: Core → Toolkit

Updated

4 months ago
Priority: -- → P3

Updated

4 months ago
See Also: → bug 1198194
You need to log in before you can comment on or make changes to this bug.