Closed Bug 473930 Opened 16 years ago Closed 15 years ago

Feeds linked in <head> with feeds:// and feedsearch:// (or any other incorrect/fake) schemes/protocols are detected and sent to the default feed reader

Categories

(Camino Graveyard :: OS Integration, defect)

Product:
Camino Graveyard
Component:
OS Integration
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: phiw2, Unassigned)

References

(
URL
)

Details

Attachments

(2 files)

fix v1.0
2.29 KB, patch
chris
: review-
Details | Diff | Splinter Review
Testcase with feeds and feedsearch in head
536 bytes, text/html
Details 
per bug 473571 comment 4

Feeds linked in the <head> of an html doc with feeds:// and feedsearch:// protocols are sent to the default feed reader

bug 473571 comment 7 stated

> If we want to protect our users from whatever this exploit is, we should do so
> by fixing the bug outlined in comment 4 (which I think we should fix regardless
> of the Safari exploit; we should only attempt to handle things starting with
> 'feed', 'http' or 'https') and take the patch in comment 2 to shut down Gecko. 
> That way we don't hand off the bogus protocols (that no one besides Safari even
> supports) to begin with, so we don't have to care what our users are using as a
> feed reader.


I should note that this happens even if I set the OS to do nothing with these protocols. Using the RCDefaultApp prefpane, I have set these to <disable>. Camino passed the feeds to NetNewsWire.

attachment (id=357129) in bug 473571 has a sample test case.
Nick, any chance you want to work on this?
Is this a Camino bug, or is this a Gecko bug? Seems like

network.protocol-handler.external.foo

should work for disabling links in the head as well as in the body of a document.

cl
(In reply to comment #2)
> Is this a Camino bug, or is this a Gecko bug?

Given we do our own feed-handling, it's most certainly ours.

How are feeds: and feedsearch: getting past http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/camino/src/embedding/CHBrowserListener.mm&rev=1.56&mark=892#887 ?
It's not getting past that code; it's failing at that point, but moving on to the next block, where we check the 'alternate' attribute and the 'type' attribute. That code doesn't look at the protocol at all, but then again, it's based on the checks Firefox does (and Firefox doesn't look at the protocol there either).

Firefox 3, of course, detects that feed too, and the patch from the other bug doesn't affect Firefox's behaviour in that regard either.

I'd personally like to see that check move to a low enough level that client apps don't have to worry about doing it at all (i.e., this should be a Gecko bug), but we can certainly add some code to fix it ourselves if we don't want to wait.

Is a fix for this something we want to take in 1.6.7? If so, that fix will be totally different from the trunk fix.

cl
Oh, or is this something that isn't a problem in 1.6.x anyway because we had very limited feed detection there?
Hardware: x86 → All
Added philor to the CC list in the hopes that he might be able to help with this question...

Are there any URI schemes other than "feed", "http", and "https" that are legitimate schemes for feed links? I'm wondering whether it's a good idea to hardcode a check for those three schemes at the very beginning of the feed-detection code and bail if it isn't any of those, regardless of what network.protocol-handler.external.foo prefs are set.

I still think those prefs should prevent handing off URIs with those schemes to any external app, though. There are two different bugs at work here:

1) We detect feeds: and feedsearch: links as feeds, and we shouldn't. That's something we can fix in our code fairly easily.

2) Those links are still handed off despite the pref being set to prevent it. That's something that needs to be handled somewhere in Gecko, I think. It might be a bug in our code (not checking something against that list, maybe), though.
(In reply to comment #5)
> Oh, or is this something that isn't a problem in 1.6.x anyway because we had
> very limited feed detection there?

No, we detect them in 1.6.x.  Whether we want the fix there or not can wait until we get closer and we can evaluate the scope of the fix and Apple's response to their hole.  (That is, if Apple fixes their hole before 1.6.7, I might be inclined to just leave this as another bit more of all the broken feedhandling in 1.6.x.)

(In reply to comment #6)
> 2) Those links are still handed off despite the pref being set to prevent it.
> That's something that needs to be handled somewhere in Gecko, I think. It might
> be a bug in our code (not checking something against that list, maybe), though.

Well, we're handing off "feed:feedsearch://example.com/atom/" to external apps, so there's no way checking for "network.protocol-handler.external.feedsearch = false" would stop that, were that pref even set.  This bug is entirely in our code somewhere.
Attached patch fix v1.0 — — Splinter Review 
This implements comment 6 by preventing anything other than http:, https:, or feed: URIs from being passed to the feed-detection code in the first place. Relative URIs are also permitted, since their schemes are implicitly http or https.

I tested this against the various testcases in bug 383527 and it doesn't regress any of them. It correctly prevents us from detecting the fake feed in the testcase in comment 0 of this bug, and should prevent us from detecting any other "feeds" with schemes other than http(s) or feed in the future.
Assignee: nobody → cl-bugs-new
Status: NEW → ASSIGNED
Attachment #357405 - Flags: review?(trendyhendy2000)
Summary: Feeds linked in <head> with feeds:// and feedsearch:// protocols are sent to the default feed reader → Feeds linked in <head> with feeds:// and feedsearch:// (or any other) schemes/protocols are detected and sent to the default feed reader
Summary: Feeds linked in <head> with feeds:// and feedsearch:// (or any other) schemes/protocols are detected and sent to the default feed reader → Feeds linked in <head> with feeds:// and feedsearch:// (or any other incorrect/fake) schemes/protocols are detected and sent to the default feed reader
[5:23pm] philor: bug 473930 really really needs a step back to clearly stating what problem it's trying to solve
[5:24pm] sauron: philor: meaning what?
[5:25pm] philor: since the current direction seems to be solving "The minor annoyance of having my feed reader get a URI with a scheme it doesn't recognize is so important that neither weird people serving feeds over ftp nor people inventing a new protocol and writing a feed reader that talks that protocol should be allowed to use autodiscovery"
[5:26pm] philor: meaning that you started out from a vaguely described exploit that means nobody should open Safari's feed display, and went from that to a patch which will not solve that, but will limit what URIs people can use for feeds
[...]
[5:34pm] philor: "the page author says that his feed URI is"
[5:36pm] philor: if you are sniffing, then false positives are your fault, but this isn't about sniffing, this is about someone explicitly saying that their feed URI is something that may or may not work with any particular feed reader
[...]
[5:47pm] philor: gotta go to work, but... that patch is both a bad idea, and *does not solve any problem*

There were about 20mins of arguing in the middle there, but Phil points out that since we're never emitting feeds: or feedsearch: URIs directly from autodiscovery (they're always feed:foo as seen in comment 7), we're not Typhoid Mary in the exploit (via autodiscovery; Gecko, OTOH, is).

There's still an argument to be made for not knowingly sending other apps 'bad' data ([5:29pm] philor: why is that bad data?) that they won't be able to handle (i.e., stuff like "feed:feedsearch://example.com/atom/" and "feed:feeds:Official%20Camino%20Weblog&weblog.philringnalda.com/feed&filter:Camino"), but Phil's argument is we can't know for sure whether apps will be able to handle that or not.  

The UE angle on the 'bad' data is that if we auto-discover these feeds, then the reader apps will either throw an error message or perhaps do nothing, and users may (will) complain about us either detecting garbage as feed, sending garbage to their reader, or failing to send data altogether (in the 'app does nothing' case, e.g. Google Reader)--just as they complain to us when a web page doesn't render 'correctly' for whatever reason.
/me backspaces

Hey, that saved me some re-typing!

Imagine for the sake of argument that you can actually query the user's feed reader somehow, and find out what URI schemes it supports. Even in that idyllic situation, do you really think the best UE would be to treat the situation where the page has told you that it has a feed but you know it's not going to work with the user's feed reader as-is as though the page hadn't told you about the feed at all?

If I go to a page, and want to subscribe to the feed for it, and there's no autodiscovery icon in the location bar, I'm going to grumble about my crappy browser missing it, or the crappy author screwing up the <link> or not having one at all, and I'll copy the link from his orange icon in the page or the linked word "Atom" at the bottom of the page or wherever I find it, and paste it into my feed reader. Among all the possibilities (the URI is something weird like iris.beep: or bogus like feeds:, the URI is a typo like htp:, which is thousands of times more likely than any other weird scheme), there's not a single one where I'll thank you for making me go the long way around.

Of course reality is much messier than that: you don't know whether or not my feed reader supports ftp: (actually, neither do I) today, and you don't know whether the feed reader I'll be using in six months together with the browser you ship today supports held+beep:. If you pass the URI that a page says is for its feed to my feed reader, and it does something stupid with it, that's a bug in my feed reader. If you fail to admit to seeing a perfectly correctly formed autodiscovery <link> because it wouldn't end up working in _your_ feed reader, that's a bug in your browser.

As far as security and the current exploit: doing something to protect your users from that, assuming you think you'll have an update before Apple does, is perfectly reasonable, but this will do absolutely nothing for that. The only people who will get an autodiscovered <link> sent to Safari are ones who have it still set to handle feed:, so there's no point in anyone trying a malicious <link href="feedsearch:..."> when it won't catch any more people than href="feed:..." will. For autodiscovery, refusing to pass anything at all to Safari if it's the selected handler is reasonable; for feeds: and feedsearch:, the bug 473571 approach of (at least temporarily, since Safari doesn't really deserve to own those two words for all time and all purposes) refusing to send them anywhere, since you don't have control over vetoing the user's choice in a protocol handler dialog, is also reasonable.
(In reply to comment #10)
> Among all the possibilities (the URI is something weird
> like iris.beep: or bogus like feeds:, the URI is a typo like htp:, which is
> thousands of times more likely than any other weird scheme), there's not a
> single one where I'll thank you for making me go the long way around.

That's a good point (particularly the htp: part).

Sam and I talked some after you'd gone to work, and I think that both he and I are both now leaning towards doing nothing with respect to this "bug" barring new information (or lots of user complaints, of which there have been about 1 so far, about passing 'feed:feeds:Official%20Camino%20Weblog&weblog.philringnalda.com/feed&filter:Camino' and friends to their feed readers).
Testcase html doc with link elements in the head to feeds. Neither the feeds:// nor the feedsearch:// should be detected, but with Fix v1.0 the former still is. I think EqualsIgnoresCase is the culprit, but can't be sure.

Do we even want to do this? I haven't been following the conversations that closely.
Attachment #357405 - Flags: review?(trendyhendy2000) → review-
Comment on attachment 357405 [details] [diff] [review]
fix v1.0

See comments in the testcase attachment.
(In reply to comment #12)

> Do we even want to do this? I haven't been following the conversations that
> closely.

Despite my earlier assertion it needed to be filed and fixed, no, probably not.
Yeah, I'm cool with WONTFIXing this, too, since we aren't actually passing out the bogus schemes anywhere. Sorry for the noise :)
Assignee: cl-bugs-new → nobody
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: