Bug 24300 – Keep visited links private so that history info isn't leaked.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Robert Hogan]

http://ha.ckers.org/weird/CSS-history.cgi demonstrates how malicious websites can use the 'visited' element in CSS stylesheets to inspect the user's recent browsing history. 

The attached patch introduces a webcore setting 'CSSKeepVisitedLinksPrivate' that allows webkit browsers to disable this behaviour.

[Robert Hogan]

Created attachment 28196 [details]
CSSKeepVisitedLinksPrivate patch

Tested with Arora.

[Eric Seidel (no email)]

Comment on attachment 28196 [details]
CSSKeepVisitedLinksPrivate patch

This also disables display of :visited styles, correct?  If so, this needs a better setting name.  Like visitedPseudoStyleEnabled().

[Robert Hogan]

Created attachment 31664 [details]
Updated patch

Still unsure of what to call the option but have altered it to the more explicit 'CSSIgnoreVisitedLinks'.
Revisions include:
 - addition of a manual test-case
 - the original patch wasn't working properly - this one has been thoroughly tested
 - um, that's it.

[Adam Barth]

Comment on attachment 31664 [details]
Updated patch

> +    Settings* settings = m_document->settings();
> +    if (!settings || settings->cssIgnoreVisitedLinks())
> +        m_IgnoreVisitedLinks = true;

This should default to false if we can't find settings.

> +++ b/WebCore/manual-tests/css-ignore-visited-links.html

This should be a LayoutTest instead of a manual test.  See something like xssAuditor to see how to test non-default settings in LayoutTests.

[Robert Hogan]

Created attachment 31746 [details]
Updated patch with layout test

m_IgnoreVisitedLinks is already set to false by default by the constructor. won't that do the trick?

i've added a layout test - had to add a stub history manager to the qt dumprendertree to support it.

[Adam Barth]

Comment on attachment 31746 [details]
Updated patch with layout test

This is looking good!  Below are some minor style issues.  Once we address those, we're all set.

> +function runTest() {
> +    if (window.layoutTestController) {
> +        layoutTestController.notifyDone();
> +    }
> +}

No braces around one-line if statements.

> +    Settings* settings = m_document->settings();
> +    if (!settings || settings->cssIgnoreVisitedLinks())
> +        m_IgnoreVisitedLinks = true;

If |settings| is null, then m_IgnoreVisitedLinks gets set to true, which is isn't what we want (as I said in my previous review).

> @@ -262,7 +265,6 @@ QString DumpRenderTree::dumpFramesAsText(QWebFrame* frame)
>  
>  QString DumpRenderTree::dumpBackForwardList()
>  {
> -    QString result;
>      result.append(QLatin1String("\n============== Back Forward List ==============\n"));
>      result.append(QLatin1String("FIXME: Unimplemented!\n"));
>      result.append(QLatin1String("===============================================\n"));

I don't understand why you removed this line.  Is this just unrelated cleanup?

>      QWebPage *m_page;
> +    HistoryManager *historyManager;

These * are misplaced.  I see that you're matching the style of this file, however.

> +HistoryManager::HistoryManager(QObject *parent)
> +    : QWebHistoryInterface(parent)
> +    ,m_count(0)

Missing a space after the , here.

> +    m_count++;

Prefer pre-increment (e.g, ++m_count).

Will this test fail on other platforms since you only implemented this API for QT?  Maybe you should update the skipped test lists for those platforms to avoid turning the tree red?

[Robert Hogan]

Created attachment 31753 [details]
Updated patch with suggested changes

oops - couple of clangers in there. thanks!

have ignored 2 items - retained the style in dumprendertree.h (because the rest of the file is like that as you note) and the unit-tests are already in qt's platform specific folder.

[Adam Barth]

Comment on attachment 31753 [details]
Updated patch with suggested changes

Awesome!  Thanks for fixing the nits.

[Sam Weinig]

I don't think the license in patch in (for historymanager.cpp) is one of the ones we accept for patches in WebKit.  I would suggest not landing this change until it has been worked out.

[Eric Seidel (no email)]

Comment on attachment 31753 [details]
Updated patch with suggested changes

This patch is not BSD or LGPL.  Rolling out.  It should never have been r+'d.

[Eric Seidel (no email)]

Oh, nm.  I never landed it. :)  So not rolling it out... but r-ing it. :)

[Maciej Stachowiak]

Comment on attachment 31753 [details]
Updated patch with suggested changes

This patch includes GPL-licensed code, so it can't be licensed as-is. WebKit code has to be LGPL or BSD-style licensed. Changing to r-.

[Robert Hogan]

Created attachment 31797 [details]
Patch with Update Copyright Notices

I've updated the copyright notice on historymanger.* and also tweaked a few bits. 
Just FYI I started out with http://github.com/Arora/arora/tree/master/src/history/historymanager.cpp and ended up removing everything except the minimum required to support the tests. The contents of the remaining functions are entirely different from the original and their names are defined by qwebhistoryinterface.

[Adam Barth]

Comment on attachment 31797 [details]
Patch with Update Copyright Notices

(In reply to comment #13)
> Just FYI I started out with
> http://github.com/Arora/arora/tree/master/src/history/historymanager.cpp

INAL, but it sounds like this patch is a derived work from the above URI and therefore subject to the GPL.  Are you sure you have the legal right to relicense the code?  (I'm pretty sure that's what you warrant when you click through the patch upload agreement.)

[Robert Hogan]

(In reply to comment #14)
> (From update of attachment 31797 [details] [review])
> (In reply to comment #13)
> > Just FYI I started out with
> > http://github.com/Arora/arora/tree/master/src/history/historymanager.cpp
> INAL, but it sounds like this patch is a derived work from the above URI and
> therefore subject to the GPL.  Are you sure you have the legal right to
> relicense the code?  (I'm pretty sure that's what you warrant when you click
> through the patch upload agreement.)

I believe I have ended up with a completely different class - in fact it is a minimal class required to support qwebhistoryinterface. I understand that the way I wrote the class has created ambiguity, so you are right to be cautious.

At this point the only common elements between my historymanager.cpp and the one in Arora are:

 - the filenames
 - the name of the class (HistoryManager)

The only function names it shares with arora's are the ones that it is required to reimplement from qwebhistoryinterface in QtWebKit in order to function - addHistoryEntry and historyContains. The contents of both these functions is different from the arora file.

Can you advise what I need to do dispel the whiff of sulphur here?

[Adam Barth]

> Can you advise what I need to do dispel the whiff of sulphur here?

That question is beyond my pay grade...  Hopefully someone with more knowledge of these issues will chime in.

[Benjamin Meyer]

I reviewed the patch and did not find it to be a derivative of (my) Arora's history manager.  Any subclass of the history interface will look similar to both this and Arora's.

[Robert Hogan]

Created attachment 31865 [details]
New class DRTHistory replacing HistoryManager

If it helps,  I've updated the patch with two completely new files drthistory.cpp and drthistory.h and removed historymanager.cpp and historymanager..  The class they implement is now called DRTHistory.  I'll also submit a diff between the arora historymanager.cpp/h and drthistory.cpp/h. Hope this isn't OTT but just documenting that they are demonstrably non-derivative.

[Robert Hogan]

Created attachment 31867 [details]
diff

as mentioned in previous comment

[Eric Seidel (no email)]

Comment on attachment 31865 [details]
New class DRTHistory replacing HistoryManager

Don't you want to return PseudoAnyLink instead of PseudoLink?  If so, it seems the tests need to be updated to catch that case.

Otherwise this looks fine.

[Dave Hyatt]

Comment on attachment 31865 [details]
New class DRTHistory replacing HistoryManager

if (!m_ignoreVisitedLinks && pseudoState == PseudoVisited)

Why is the above change necessary?  If you hack checkPseudoState to always pretend like you aren't visited, then why do you need this extra code?

[Robert Hogan]

(In reply to comment #21)
> (From update of attachment 31865 [details] [review])
> if (!m_ignoreVisitedLinks && pseudoState == PseudoVisited)
> 
> Why is the above change necessary?  If you hack checkPseudoState to always
> pretend like you aren't visited, then why do you need this extra code?
> 
You're right - this line is redundant. I've tested it without it, and manual and auto tests still pass. I think it was introduced in an earlier stage in the patch, when for some reason I had the check in checkPseudoState in the wrong place.

[Robert Hogan]

Created attachment 32098 [details]
Updated patch

Updated to remove redundant check.

[Eric Seidel (no email)]

Comment on attachment 32098 [details]
Updated patch

I thought the QWebHistoryInterface interface was deprecated?  I know that WebKit qt has some hacks in place around the WebCore link hashing system just to support this interface.  Seeems we should not be using it from DRT if something better is available.

Style:
 92     DRTHistory *historyManager;

Leaked:
 154     historyManager = new DRTHistory(m_page);

You tests shoudl ideally e dumpAsText, since all they need to do is check the computedStyle, they don't need to dump the pixels for the page to confirm this change.

[Kenneth Rohde Christiansen]

AFAIK, we didn't deprecate QWebHistoryInterface as we have no other history API in place. If we should do so, now would be the time before our Qt 4.6 release.

CC'ing Simon to see if he has some input.

[Robert Hogan]

Created attachment 34405 [details]
Updated patch

Thanks Eric - patch updated per your comments. Hopefully this patch will do if it's decided that moving from the deprecated QWebHistoryInterface API is another day's work.

[Eric Seidel (no email)]

It seems from this commit that ripping this bad API out of Qt was slated for 4.5... I guess that never happened?

http://trac.webkit.org/changeset/43052

[Holger Freyther]

(In reply to comment #27)
> It seems from this commit that ripping this bad API out of Qt was slated for
> 4.5... I guess that never happened?

It will not happen. The API needs to stay like this until Qt5.0. What Nokia can and should do (note I'm not with Nokia) is to create a new API that follows the "push" history into WebCore model and deprecate the old one.

[Kent Tamura]

FYI: a similar bug entry for Mozilla:
  https://bugzilla.mozilla.org/show_bug.cgi?id=147777
They have discussed this issue for 7 years.

[Eric Seidel (no email)]

Comment on attachment 34405 [details]
Updated patch

Does qt have support yet for the layoutTestController.overridePreference stuff?  If so, this test really should use that and not be qt-specific (skipped by all the other platforms if they don't want to add this preference).

I think this patch is OK.  Just not very excited about it.  Especially with it using qt API which we were hoping to see die some day. ;)

[Robert Hogan]

> (From update of attachment 34405 [details])
> Does qt have support yet for the layoutTestController.overridePreference stuff?
>  If so, this test really should use that and not be qt-specific (skipped by all
> the other platforms if they don't want to add this preference).

Unfortunately qt does not support overridePreference at the moment.

> 
> I think this patch is OK.  Just not very excited about it.  Especially with it
> using qt API which we were hoping to see die some day. ;)

In the patch's defence, it is adding support for the current qt history API to qt's DRT in order to permit an autotest. The feature itself doesn't depend on this API. Does that make a difference?

[Simon Hausmann]

I admit I feel a certain hesitation of adding a feature to the QtWebKit API that isn't used by any other port (despite the fact that it appears useful).

So if all this is solved on a different level in the future, then this setting in the API would become obsolete I guess?

[Robert Hogan]

(In reply to comment #32)
> I admit I feel a certain hesitation of adding a feature to the QtWebKit API
> that isn't used by any other port (despite the fact that it appears useful).

I think there are people from other ports cc'd on the bug so they can chime in if they intend to use the setting. Perhaps there is usually some consensus built with other parts before adding a setting to webcore? It's there if they want to use it I guess. It might be a natural fit with the 'Private Browsing' mode offered by some browsers.

> 
> So if all this is solved on a different level in the future, then this setting
> in the API would become obsolete I guess?

Yes, it would. Solving it without a setting - i.e. allowing the CSS style to display normally without leaking the information remotely - appears to be an intractable problem at this point. But if a solution is ever found, then it would make this setting obsolete.

[Adam Barth]

Comment on attachment 34405 [details]
Updated patch

This patch seems fine, but please add Preference APIs for the other platforms so we can have better test coverage of this feature.  Also m_ignoreVisitedLinks isn't the greatest name.  We're not ignoring visited links.  We're ignoring the history for the purpose of computing the :visited pseud-selector.

[Robert Hogan]

(In reply to comment #34)
> (From update of attachment 34405 [details])
> This patch seems fine, but please add Preference APIs for the other platforms
> so we can have better test coverage of this feature.  

I will be able to add and test it for gtk and possibly win and wx, but I don't have the build environment for mac, so let me know if that's not enough.

> Also m_ignoreVisitedLinks
> isn't the greatest name.  We're not ignoring visited links.  We're ignoring the
> history for the purpose of computing the :visited pseud-selector.

OK. m_ignoreHistoryForCSS ? (I'm bad at names as you can tell)

[Robert Hogan]

Created attachment 44334 [details]
Update Patch

Updated patch adding preference and unit tests to gtk, qt, and win ports.

[Simon Hausmann]

For the Qt port I would prefer a positive naming of the attribute in QWebSettings, i.e. FooEnabled with a default to true - simply for consistency.

What do the other Qt hackers think? :)

[Robert Hogan]

Created attachment 44360 [details]
updated patch

Updated per Simon's comments - option is now CSSVisitedStyleEnabled and set to true by default for all platforms.

[WebKit Review Bot]

Attachment 44360 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebsettings.cpp:101:  css_visited_style_enabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/webkit/webkitwebview.cpp:2548:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 2

[Robert Hogan]

Created attachment 44364 [details]
Update to skip test for Mac

[Darin Adler]

Comment on attachment 44364 [details]
Update to skip test for Mac

> @@ -873,7 +873,11 @@ CSSStyleSelector::SelectorChecker::SelectorChecker(Document* document, bool stri
>      , m_collectRulesOnly(false)
>      , m_pseudoStyle(NOPSEUDO)
>      , m_documentIsHTML(document->isHTMLDocument())
> +    , m_cssVisitedStyleEnabled(true)

In a class named CSSStyleSelector, it seems that include "css" in the name of the data member is not helpful. The best boolean data member names are things that complete a phrase "style selector <xxx>" so I would call this m_ignoresVisitedLinks.

I'm also not sure the CSS style selector is the right level for this. The PageGroup class already has a function setShouldTrackVisitedLinks. Do we really need to add something else?

I'm going to say review- for now so you can consider both of those comments.

[Robert Hogan]

(In reply to comment #41)
> (From update of attachment 44364 [details])
> > @@ -873,7 +873,11 @@ CSSStyleSelector::SelectorChecker::SelectorChecker(Document* document, bool stri
> >      , m_collectRulesOnly(false)
> >      , m_pseudoStyle(NOPSEUDO)
> >      , m_documentIsHTML(document->isHTMLDocument())
> > +    , m_cssVisitedStyleEnabled(true)
> 
> In a class named CSSStyleSelector, it seems that include "css" in the name of
> the data member is not helpful. The best boolean data member names are things
> that complete a phrase "style selector <xxx>" so I would call this
> m_ignoresVisitedLinks.
> 
From comment #34:
> Also m_ignoreVisitedLinks
> isn't the greatest name.  We're not ignoring visited links.  We're ignoring the
> history for the purpose of computing the :visited pseud-selector.
:-) I think you read that comment and are disagreeing with Adam - so I'll change it back again if no one still objects.. or let me know.

> I'm also not sure the CSS style selector is the right level for this. The
> PageGroup class already has a function setShouldTrackVisitedLinks. Do we really
> need to add something else?
> 
I should have mentioned in the original bug report that I decided against this because the purpose of the setting is to conceal visited links from CSS exploits rather than not track visited links at all (and wipe from the hash db any already visited). This seemed the less intrusive way of implementing it, since the user may find it confusing when they switch *out* of the option and find that links which were displayed as visited before they switched are now displayed as not-visited.


> I'm going to say review- for now so you can consider both of those comments.

[Robert Hogan]

Comment on attachment 44364 [details]
Update to skip test for Mac

See comment #42

[Eric Seidel (no email)]

Comment on attachment 44364 [details]
Update to skip test for Mac

The EWS builders seem to think this patch does not apply.  Meaning it won't be able to be cq'd even if r+'d.

[Eric Seidel (no email)]

http://webkit-commit-queue.appspot.com/patch/44364 is the EWS status link btw.

[Eric Seidel (no email)]

Comment on attachment 44364 [details]
Update to skip test for Mac

This patch does not apply and appears abandoned.  r-

[Robert Hogan]

Created attachment 48915 [details]
Updated Patch

God loves a trier!

[Eric Seidel (no email)]

This  new patch also does not apply, as you can tell by the purple bubbles below the patch.  They should be green (except for Mac) if your patch applies correctly and builds.

[Robert Hogan]

Created attachment 49155 [details]
Updated Patch

[WebKit Review Bot]

Attachment 49155 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebsettings.cpp:1200:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Ignoring "WebKit/qt/Api/qwebsettings.cpp": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebsettings.h": this file is exempt from the style guide.
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Hogan]

Created attachment 49157 [details]
Updated Patch

[Adam Barth]

Comment on attachment 49157 [details]
Updated Patch

I haven't discussed this with other members of the project, but I think we should fix this by default instead of adding an option.

One approach that can work by default is the one outlined here:
http://dbaron.org/mozilla/visited-privacy

[Dave Hyatt]

I'm going to implement dbaron's solution to this problem.  It's great.  That way we don't need an extra option for this.

[Dave Hyatt]

Taking the bug.

[Dave Hyatt]

Another consideration on top of what dbaron did is that if we ever allow for the drawing of HTML elements into a canvas, we will need to have a notion that the paint is "untrusted", and only paint the unvisited styles.  For now I'll punt on that, but I'll put a FIXME in the drawing function that will mention this future issue.

[Dave Hyatt]

Created attachment 52338 [details]
Work in progress.

[Dave Hyatt]

Created attachment 52436 [details]
This patch gets all the drawing code changed properly (except for SVG fill and stroke).

This patch gets all the drawing code for border, outlines, column rules, etc. patched properly.  SVG fill and stroke still need to be patched.

visitedDependentColor right now still just returns the unvisited style color, so the next step is to patch it to really fetch the visited style.  It will do this for both visited and unvisited links to prevent timing attacks based off slow style computation.

[Dave Hyatt]

Created attachment 52451 [details]
Fix collapsed border drawing to propagate colors from the original elements participating in the collapse

To get collapsed borders in tables working, I had to fix bugs with color propagation in those functions.  Now a CollapsedBorderValue will make sure to bring along the color property from the original element if no border color is explicitly set.  The current code in TOT always uses the cell's current color when no border color is set on the winning border style, and that's wrong.

[Adam Barth]

dhyatt and I chatted about timing attacks on #webkit the other day.  dbaron has a timing testing that you might be interested in testing your patch against:

https://bugzilla.mozilla.org/show_bug.cgi?id=147777#c35

[Dave Hyatt]

Created attachment 52552 [details]
This patch gets the feature working!

[Dave Hyatt]

Created attachment 52572 [details]
Add SVG fill/stroke support

[Dave Hyatt]

Some notes:

My biggest worry is performance (and to a lesser extent footprint).  All links and their descendants effectively have two styles.  The time to compute the style information for these objects has doubled, and the style footprint for these objects has doubled.

I'm not actually computing an unvisited and a visited style for each node.  I'm computing an unvisited and real style for each node.  If the real style is also unvisited then you end up with duplicated styles.  It might be better to compute a forced visited style for each node, so that the two styles are actually unvisited and visited.

Doing so means I'd need to know if the link is actually visited at paint time rather than just always using the visited color.

I pretty much have to do this to avoid timing attacks though.

[Dave Hyatt]

Created attachment 52582 [details]
Ready for a first review pass

This patch is ready for someone to take a look at it.

It's kind of an open question how we want to do tests.  This feature is extremely difficult to test using our layout test system, so I need to think about what the best way to test it is.

[Adam Barth]

Isn't this trivial to test with pixel tests?

[WebKit Review Bot]

Attachment 52582 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/style/RenderStyle.cpp:502:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.h:178:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.h:204:  _insideLink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/style/RenderStyle.h:249:  _styleType is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/style/RenderStyle.h:255:  _isLink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderObject.cpp:828:  Missing spaces around /  [whitespace/operators] [3]
WebCore/rendering/RenderObject.cpp:834:  Missing spaces around /  [whitespace/operators] [3]
WebCore/rendering/RenderObject.cpp:2302:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderObject.cpp:2303:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/InlineFlowBox.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/LinkHash.cpp:194:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 11 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 52582 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1651197

[Alexey Proskuryakov]

<rdar://problem/6407151>

[Dave Hyatt]

Created attachment 52738 [details]
Patch that fixes link hashing for <a href="">

[Adam Roben (:aroben)]

Comment on attachment 52738 [details]
Patch that fixes link hashing for <a href="">

The code change looks good. But no regression test makes me sad!

[Dave Hyatt]

Created attachment 52778 [details]
Patch including ChangeLogs and tests

[Oliver Hunt]

Comment on attachment 52778 [details]
Patch including ChangeLogs and tests

r=me, but you need to fix the issues i mentioned on irc: correct bit count, windows drt stuff in particular

[Dave Hyatt]

Fixed in r57292.