Bug 371179 lists some dangers that I don't quite follow that arise from javascript: and data: URLs being bookmarked behind the user's back. The fix Firefox seems to be implementing is to prevent Bookmark Current Page(…) and Bookmark Current Tabs as Tab Group(…) from bookmarking javascript: and data: URLs. Flagging 1.1? since there's security implications; this should be fairly easy to fix with our isRealLiveURI stuff, right?

Yeah, this shouldn't be hard to fix. I still don't entirely understand why it's bad, or whether we need to prevent it from ever happening (via a manual edit of the bookmark later) or if preventing it from happening on bookmark creation is enough. cl

As we consider how to do this, we should also consider adding a dialog when trying to bookmark such URIs. See bug 371923. I definitely agree that it'd be a good idea to do so. I'm not sure if that's a string that will come from core or not, but we could easily do it on our end. If we did choose to add a dialog, that will break our string freeze. On that presumption, until we decide otherwise, I'm adding the late-l10n keyword.

Per irc: [12:52pm] pinkerton: why not just disable the "bookmark item" and drop the tab from "bookmark all" [12:52pm] pinkerton: done.

murph said we're going to need this change in header search paths for this....

I'll implement comment 3. We're going to attempt utilizing Core's nsIScriptSecurityManager to rely on existing functionality and provide future-proof url validation as security requirements evolve.

Implements comment 3. I added a new method to BrowserWrapper entitled |isSafeURI| which will utilize nsIScriptSecurityManager to determine if any security implications arise from the current URI of the wrapper/BrowserView. The following techniques of bookmarking a single page are disabled: - Page context menu: Bookmark Current Page - Main Menu: Bookmark Current Page… - Main Menu: Bookmark Current Page (without prompt) Tab group techniques which drop unsafe URIs: - Main Menu: Bookmark Current Tabs as Tab Group… - Main Menu: Bookmark Current Tabs as Tab Group (without prompt) - Bookmark Manager: Clicking the "Plus" pop-up button and selecting Add Bookmark…, and then selecting the Bookmark as tab group option The creation of tab groups will simply drop any unsafe URIs without user intervention. The default title string in the Add Bookmark dialog acknowledges if any were dropped and displays an accurate count (such as "[3 tabs] Primary Tab Title"). The Main Menu item "Bookmark Current Page" is disabled via automatic menu validation. The validation routine is called upon by the Cocoa runtime very frequently, so we need to be sure the isSafeURI method as part of that validation is efficient enough to handle this heavy usage. Some explanation/notes on the code: The two tab group methods, |showAddBookmarkDialogForAllTabs:| and |addTabGroupWithoutPrompt:|, had both been using a very similar approach to gather up the current tabs for bookmarking. There was enough repeated code between them that I thought it might be advantageous to abstract away the tab aggregation into its own method. This meant that I didn't have to repeatedly place isSafeURI checks in multiple places just to deal with the single issue of creating tab groups. I introduced a helper method, |bookmarkableTabItems|, which returns all current tab items suitable for bookmarking in the keyed-format utilized by AddBookmarkDialogController (ABDC). I chose to use ABDC format because |showAddBookmarkDialogForAllTabs:| can pass the returned array directly to the dialog controller, plus it avoids having to reinvent and use another NSArray/NSDictionary format. While this keyed format was useful to |showAddBookmarkDialogForAllTabs:|, the other method |addTabGroupWithoutPrompt:| does not require the current tabs information to exist in the manner specified by ABDC. Nevertheless, doing so didn't seem to be a major disadvantage. I needed a way to package up that information anyway. AddBookmarkDialogController already contained several class methods for accessing information contained in an array of tab items. These were previously private, but their usefulness to |addTabGroupWithoutPrompt:| caused me to move them over to the header and make them public instead. This allowed |addTabGroupWithoutPrompt:| to easily access tab group information with less code. The refactoring in regard to tab groups is not necessary to implement this bug, but it simplified some methods and took away some code. Also, it's difficult to grasp the changes I made by only looking at the diff, so manually comparing the methods of interest might be a good idea. This patch does not validate URI security in two situations: - drag and drop creation of bookmarks - adding bookmarks from the link (<a href>) context menu item "Bookmark This Link…" Should these cases be validated as well? We have to be careful here not to break the behavior of javascript: bookmarklets (http://en.wikipedia.org/wiki/Bookmarklets), which is why I didn't yet prevent the two above situations. Those might be the two most common ways users might add a bookmarklet. As is typical with increased security, if we start to get in the way of desired behavior too much, especially with bookmarklets, we might have to just present a warning dialog (like ss mentioned in comment 2) and ensure the user is fully aware of the URI and possible dangers. I'll gladly add support for the two cases above if deemed necessary. The isSafeURI method currently audits the current CHBrowserView URI, and will have to be changes in these two situations to take a URI argument, since it's not yet loaded in anything. FYI: There are some useful and safe data: based URIs that makes testing easier at <http://www.mozilla.org/quality/networking/testing/datatests.html> and a good demonstration of the dangers these types of URIs expose is at <http://lcamtuf.coredump.cx/ffbook/>.

Comment on attachment 258349 [details] [diff] [review] patch > - (IBAction)addTabGroupWithoutPrompt:(id)aSender > { > BookmarkManager* bookmarkManager = [BookmarkManager sharedBookmarkManager]; >- BookmarkFolder* parentFolder = [bookmarkManager lastUsedBookmarkFolder]; >+ BookmarkFolder* parentFolder = [bookmarkManager lastUsedBookmarkFolder]; >+ NSArray* tabItems = [self bookmarkableTabItems]; Going through the tabs and packing them into an array of dictionaries whose format is only meaningful to AddBookmarkDialogController, then unrolling it back out to call addBookmark:url:inPosition:isSpearator: on each one doesn't seem like a win, so I don't think this change should go in. >Index: src/bookmarks/AddBookmarkDialogController.h ... >++ (NSString*)bookmarkUrlForItem:(NSDictionary*)inItem; >++ (NSString*)bookmarkTitleForItem:(NSDictionary*)inItem; >++ (NSDictionary*)primaryBookmarkItem:(NSArray*)inItems; While I understand the motivation behind this, making ABDC a generic utility class for doing bookmarking even when it has nothing to do with the dialog isn't the right approach. I'd rather see common functionality moved elsewhere so that ABDC can stay focused on what it is for. (And any cleanup like that should happen post-1.1 at this point.)

In hindsight, the illogical behavior in the previous patch is obvious now. I should have realized that when the code necessitated such a detailed explanation as to why it was implemented that way! Here is a new patch which avoids getting heavy into any of that refactoring; one that focuses more on just fixing the issue at hand. We currently disable the "Bookmark Current Page…" menu item if the loaded page is empty (about:blank). It is possible though to open a window with, say, two empty tabs and "Bookmark Current Tabs as Tab Group…". So, basically, "Bookmark Current Tabs as Tab Group" used to enable itself whenever there was more than one tab, regardless of the empty/unsafe characteristics of the associated URIs. I took the menu validation a step further to prevent such a situation, ensuring that the bookmarking of tab groups is only permitted if there are multiple bookmarkable tabs (bookmarkable meaning appropriate and safe for bookmarking). This also avoids displaying the tab group dialog and later filtering out all URIs because they were insecure. I added a |numberOfBookmarkableTabViewItems| method to BrowserTabView to supplement the AppKit provided |numberOfTabViewItems|. The behavior of this method seems to fit within the context of that class, as BrowserTabView did seem to have other methods which dealt with URIs and other BrowserWrapper properties. If this method doesn't belong in BrowserTabView and that object is strictly segregated into the view layer with no concern over the browser view it contains, this method can be moved down into a controller class (BWC). Again, Cocoa's automatic menu validation is called very frequently. It doesn't appear that constant messaging to |numberOfBookmarkableTabViewItems| as part of this validation drags performance in any way. There were many places in the code where I found myself inserting the expression: (![browserWrapper isEmpty] && [browserWrapper isSafeURI]). In case our definition of a "bookmarkable" URI ever changes in the future, I wanted to encapsulate that determination and allow for only one place in which it can be modified: |isBookmarkableURI|. With all of this menu disabling and unannounced dropping of urls from tab groups, I don't want users to perceive that our menu validation and bookmarking behavior is broken. Does anyone think it will be a problem? Note: The use of |NS_SUCCEEDED(uriIsSafe)| to check the return value of nsISSM's checkLoadURIWithPrincipal() will generate a warning log message (debug builds only): "WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file .../mozilla/caps/src/nsScriptSecurityManager.cpp, line 1260" I think this adds some unnecessary noise to the already busy run log, but nevertheless most of the Core core I looked at which utilizes this SSI method checks the return value that way.

Comment on attachment 258934 [details] [diff] [review] patch 2 >+ if ([browserWrapper isBookmarkableURI]) { Rather than indenting the entire loop body, do if (![browserWrapper isBookmarkableURI]) continue; > for (unsigned int i = 0; i < numItems; i++) { > browserWrapper = (BrowserWrapper*)[[mTabBrowser tabViewItemAtIndex:i] view]; >- NSString* itemTitle = [browserWrapper pageTitle]; >- NSString* itemURL = [browserWrapper currentURI]; >+ if ([browserWrapper isBookmarkableURI]) { Same here. >+- (BOOL)isSafeURI; I don't think this method should exist; "safe" is not a clearly-defined concept. It's unsafe only to perform certain actions on them in certain contexts. Just fold the implementation into isBookmarkable >+- (BOOL)isBookmarkableURI; Speaking of which, just make this isBookmarkable. We don't need the suffix to make the meaning clear (as with isInternalURI). >+// >+// -isSafeURI >+// >+// Returns YES if the current URI could have potential security implications >+// as determined by nsIScriptSecurityManager's DISALLOW_INHERIT_PRINCIPAL. >+// (examples of such URIs include |javascript:| or |data:|) >+// >+- (BOOL)isSafeURI >+{ >+ nsCOMPtr<nsIDOMWindow> domWindow = [mBrowserView getContentWindow]; >+ if (!domWindow) >+ return NO; >+ nsCOMPtr<nsIDOMDocument> domDocument; >+ domWindow->GetDocument(getter_AddRefs(domDocument)); >+ if (!domDocument) >+ return NO; >+ nsCOMPtr<nsIDocument> document (do_QueryInterface(domDocument)); >+ if (!document) >+ return NO; >+ >+ nsCOMPtr<nsIScriptSecurityManager> scriptSecurityManager (do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID)); >+ if (!scriptSecurityManager) >+ return NO; >+ >+ nsresult uriIsSafe = >+ scriptSecurityManager->CheckLoadURIWithPrincipal(document->NodePrincipal(), >+ document->GetDocumentURI(), >+ nsIScriptSecurityManager::DISALLOW_INHERIT_PRINCIPAL); >+ >+ return (NS_SUCCEEDED(uriIsSafe) ? YES : NO); >+} >+ >+// >+// -isBookmarkableURI >+// >+// Returns YES if the current URI is appropriate and safe for bookmarking. >+// >+- (BOOL)isBookmarkableURI >+{ >+ return (![self isEmpty] && [self isSafeURI]); >+} >+ > - (BOOL)canReload > { > NSString* curURI = [[self currentURI] lowercaseString]; > return (![self isEmpty] && > !([curURI isEqualToString:@"about:bookmarks"] || > [curURI isEqualToString:@"about:history"] || > [curURI isEqualToString:@"about:config"])); > } >Index: src/browser/BrowserTabView.h >=================================================================== >RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabView.h,v >retrieving revision 1.16 >diff -u -8 -r1.16 BrowserTabView.h >--- src/browser/BrowserTabView.h 9 Feb 2007 17:49:20 -0000 1.16 >+++ src/browser/BrowserTabView.h 18 Mar 2007 04:47:30 -0000 >@@ -64,16 +64,17 @@ > // default is hide when displaying only one tab. > - (BOOL)barAlwaysVisible; > - (void)setBarAlwaysVisible:(BOOL)newSetting; > > - (void)addTabForURL:(NSString*)aURL referrer:(NSString*)aReferrer inBackground:(BOOL)inBackground; > > - (BOOL)tabsVisible; > >+- (int)numberOfBookmarkableTabViewItems; > - (int)indexOfTabViewItemWithURL:(NSString*)aURL; > - (BrowserTabViewItem*)itemWithTag:(int)tag; > - (void)refreshTabBar:(BOOL)rebuild; > - (void)refreshTab:(BrowserTabViewItem*)tab; > - (BOOL)isVisible; > // inform the view that it will be shown or hidden; e.g. prior to showing or hiding the bookmarks > - (void)setVisible:(BOOL)show; > - (void)windowClosed; >Index: src/browser/BrowserTabView.mm >=================================================================== >RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabView.mm,v >retrieving revision 1.48 >diff -u -8 -r1.48 BrowserTabView.mm >--- src/browser/BrowserTabView.mm 13 Feb 2007 17:44:29 -0000 1.48 >+++ src/browser/BrowserTabView.mm 18 Mar 2007 04:47:30 -0000 >@@ -191,16 +191,31 @@ > { > return tabItem; > } > } > > return nil; > } > >+// Returns the number tabs, excluding ones which contain a URI unsuitable for >+// bookmarking (e.g. a blank or unsafe URI) >+- (int)numberOfBookmarkableTabViewItems >+{ >+ int numberOfBookmarkableTabs = 0; >+ int numberOfTabs = [self numberOfTabViewItems]; >+ for (int i = 0; i < numberOfTabs; i++) >+ { >+ BrowserWrapper* browserWrapper = (BrowserWrapper*)[[self tabViewItemAtIndex:i] view]; >+ if ([browserWrapper isBookmarkableURI]) >+ numberOfBookmarkableTabs++; >+ } >+ return numberOfBookmarkableTabs; >+} >+ > - (int)indexOfTabViewItemWithURL:(NSString*)aURL > { > // Try the selected tab first. > if ([[(BrowserWrapper*)[[self selectedTabViewItem] view] currentURI] isEqualToString:aURL]) > return [self indexOfTabViewItem:[self selectedTabViewItem]]; > // Otherwise just walk all the tabs and return the first match. > NSArray* tabViewItems = [self tabViewItems]; > for (unsigned int i = 0; i < [tabViewItems count]; i++) {

Updated to reflect comment 10. (In reply to my own comment #9) > Again, Cocoa's automatic menu validation is called very frequently. It doesn't > appear that constant messaging to |numberOfBookmarkableTabViewItems| as part of > this validation drags performance in any way. It turns out that we disable automatic menu validation for the Bookmarks menu. Manual validation is performed "whenever a window goes away, becomes main or is no longer main, and any time the number of tabs changes, the active tab changes, or any page is loaded." So, it's still fairly frequent, but not as often as when Cocoa's in charge. - (IBAction)addTabGroupWithoutPrompt:(id)aSender I discovered a small problem when adding a tab group without the dialog. The title of the tab group is normally something like "[4 tabs] Title of Selected Tab". The count of tabs for use in this title was generated before skipping over any unbookmarkable tabs, which means that the number would be inaccurate if some were indeed dropped from the group. Additionally, if the selected tab was not ultimately included, the title of the tab group would still contain its irrelevant title. I fixed these issues. This did not pertain to bookmarking a group with the dialog.

Comment on attachment 260624 [details] [diff] [review] patch 3 Looks good, just a few more small things: >+ // Verify any potential security implications as determined by nsIScriptSecurityManager's s/Verify/Check for/ >+ nsCOMPtr<nsIDocument> document (do_QueryInterface(domDocument)); No space between |document| and |(do_Q...| >+ nsCOMPtr<nsIScriptSecurityManager> scriptSecurityManager (do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID)); Same here. >+ BookmarkFolder* newGroup = [parentFolder addBookmarkFolder]; >+ [newGroup setIsGroup:YES]; > >- NSString* titleString = [NSString stringWithFormat:NSLocalizedString(@"defaultTabGroupTitle", nil), >- numItems, [browserWrapper pageTitle]]; >- BookmarkFolder* newGroup = [parentFolder addBookmarkFolder:titleString inPosition:folderPosition isGroup:YES]; Rather than adding this immediately and then setting it up, I'd prefer this be created, set up appropriately, have all the items added to it, then actually inserted into the bookmark hierarchy once at the end. >+ for (int i = 0; i < [mTabBrowser numberOfTabViewItems]; i++) { >+ BrowserWrapper* browserWrapper = (BrowserWrapper*)[[mTabBrowser tabViewItemAtIndex:i] view]; >+ if (![browserWrapper isBookmarkable]) >+ continue; ... > [newGroup addBookmark:itemTitle url:itemURL inPosition:i isSeparator:NO]; Using |i| doesn't work here anymore; the positions will be be off once something has been skipped. You'll need to keep your own counter. >+ if (browserWrapper == [self getBrowserWrapper]) Get the current browser wrapper once outside of the loop. >+ // If the active tab was dropped from the group use a generic title >+ if (!primaryTabTitle) >+ primaryTabTitle = [[newGroup objectAtIndex:0] title]; This should check for count > 0; in theory it should always be, but if something goes wrong with validation we don't want to throw an exception. There should be an assert though, like you added to the other method. Also, the comment doesn't reflect the code (first tab's title != generic title). >+ [newGroup setTitle:[NSString stringWithFormat:NSLocalizedString(@"defaultTabGroupTitle", nil), [newGroup count], primaryTabTitle]]; Break this line so it's not quite so long: [newGroup setTitle:[NSString stringWithFormat:NSLocalizedString(@"defaultTabGroupTitle", nil), [newGroup count], primaryTabTitle]];

Sorry for keeping you busy with this one Stuart and thanks for spending the time reviewing multiple iterations. This updated patch fixes everything mentioned in comment 12. One question: Do the direct references to instance variables throughout BrowserWindowController, for example referencing mTabBrowser instead of using the -getTabBrowser accessor, violate our own encapsulation to some degree?

Comment on attachment 260870 [details] [diff] [review] patch 4 Sorry, I missed a couple of tiny things: >+// Returns the number tabs, excluding ones which contain a URI unsuitable for Typo; should be "number of tabs" >+ for (int i = 0; i < numberOfTabs; i++) >+ { Brace on the same line. r=me with those changes.

Fixes those two additional issues.

Comment on attachment 260891 [details] [diff] [review] patch 4.0.1 sr=pink

Checked in on trunk and MOZILLA_1_8_BRANCH.

/builds/tinderbox/CaminoBranch/Darwin_7.9.0_Depend/mozilla/camino/src/browser/BrowserWrapper.mm: In function `BOOL -[BrowserWrapper isBookmarkable](BrowserWrapper*, objc_selector*)': /builds/tinderbox/CaminoBranch/Darwin_7.9.0_Depend/mozilla/camino/src/browser/BrowserWrapper.mm:1081: error: `NodePrincipal' undeclared (first use this function) /builds/tinderbox/CaminoBranch/Darwin_7.9.0_Depend/mozilla/camino/src/browser/BrowserWrapper.mm:1081: error: (Each undeclared identifier is reported only once for each function it appears in.) /builds/tinderbox/CaminoBranch/Darwin_7.9.0_Depend/mozilla/camino/src/browser/BrowserWrapper.mm:1083: error: `DISALLOW_INHERIT_PRINCIPAL' is not a member of type `nsIScriptSecurityManager' ** BUILD FAILED ** A little mxring shows that indeed, NodePrincipal doesn't exist on branch (i didn't dig any further). Backed out of branch until we can get a working patch up.

My fault; I'll try to come up with a branch-friendly way of fetching these necessary parameters for nsIScriptSecurityManager::CheckLoadURIWithPrincipal(). Also, back on the branch, DISALLOW_INHERIT_PRINCIPAL is not available. We're going to have to use DISALLOW_SCRIPT_OR_DATA. No behavior change will result from this, and DISALLOW_SCRIPT_OR_DATA was later aliased to DISALLOW_INHERIT_PRINCIPAL on the trunk, allowing for backward compatibility.

"patch 4.0.1" modified to work correctly on branch. Diff between the trunk and branch patches (BrowserWrapper.mm:isBookmarkable): > // Check for any potential security implications as determined by nsIScriptSecurityManager's > - // DISALLOW_INHERIT_PRINCIPAL. (e.g. |javascript:| or |data:| URIs) > + // DISALLOW_SCRIPT_OR_DATA. (e.g. |javascript:| or |data:| URIs) > nsCOMPtr<nsIDOMWindow> domWindow = [mBrowserView getContentWindow]; > if (!domWindow) > return NO; > @@ -317,9 +317,9 @@ > if (!scriptSecurityManager) > return NO; > nsresult uriIsSafe = > - scriptSecurityManager->CheckLoadURIWithPrincipal(document->NodePrincipal(), > + scriptSecurityManager->CheckLoadURIWithPrincipal(document->GetPrincipal(), > document->GetDocumentURI(), > - nsIScriptSecurityManager::DISALLOW_INHERIT_PRINCIPAL); > + nsIScriptSecurityManager::DISALLOW_SCRIPT_OR_DATA);

Comment on attachment 261274 [details] [diff] [review] Branch-friendly patch r=me, fwiw, since it works as expected. Given this is just a simple port, that's probably sufficient before requesting sr, but that's not my call....

Comment on attachment 261274 [details] [diff] [review] Branch-friendly patch sr=smorgan. Sorry I didn't try to build branch during review.

Checked in on MOZILLA_1_8_BRANCH.