Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: tjr, Assigned: hsivonen)

Tracking

({sec-want})

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox66 fixed)

Details

(Whiteboard: [adv-main66-])

Attachments

(1 attachment)

Reporter

Description

a year ago
nsSAXXMLReader is minimally exposed to Web Content (we beleive.) We could reduce our exposed-to-web-content attack surface by clipping any vestiges of that.


> <tjr> erahm: Is nsSAXXMLReader used a lot; or is it a way to parse a flavor of XML that isn't the norm?
> <@bz> It's not the norm
> <tjr> Is nsSAXXML something we coud... put telemetry on to figure out of we can remove it?
> <@bz> Looks like it's only created by constructor
> <@bz> XPCOM ctor, that is
> <@bz> https://searchfox.org/comm-central/search?q=mozilla.org%2Fsaxparser%2Fxmlreader%3B1
> <@bz> Looks like toolkit/components/feeds/FeedProcessor.js may use the SAX stuff
> <@bz> and some bits in comm-central do
> <@bz> I dunno that there's much point in sadding telemetry
> <@bz> er, adding
> <@bz> Only things with direct xpcom access can use this 
> <@bz> The in-tree ones we can just search for
> <@bz> The out-of-tree ones are not supported.
> <@bz> So if we remove the in-tree uses we can nix this code, generally speaking
> <tjr> Ah okay, so it's not even exposed to the web, that makes me feel better.
> <@bz> tjr: feed reader may well be pumping web-origin stuff into it
> <@bz> But not directly exposed, right
> <erahm> tjr: I think you got your answer right? It's roughly internal only, still slightly used but at some point we slimmed it down
> <tjr> erahm: Yea. I'm a little unsure if I should file a bug about removing it, or investigate neutering it from the feed code though... My main angle was "Hey this seems to not be used, maybe we can close off some attack surface from web content."
> <Mossop> Gijs: There is some code that is only used by the feedprocessor that it sounds like we'd like to get rid of
> <Gijs> tjr: why is the SAX stuff attack surface ?
> <Gijs> tjr: so, fwiw, I'm pretty sure feed processor also powers live bookmarks
> <Gijs> that is harder to get rid of than just feed reader
> <Gijs> because apparently, throwing people's data away without any migration is not nice.
> <Gijs> tjr: so feed reader maaaay be going away for 61
> <Gijs> live bookmarks probably not without a release of warning, maybe
> <nemo> nooooo
> <nemo> not live bookmarks *sniff*
> <Gijs> tjr: anyway, back to my point - I expect that you could make the sax stuff not-web-exposed without breaking feeds
> <Gijs> tjr: if there is sufficient reason to do that, you may want to go ahead and do it.
> <Gijs> tjr: I will happily help find a reviewer.
> <tjr> Yea. I'm going to open a bug about it, and maybe find time to dig in :)
> — Gijs probably isn't a suitable reviewer for that change per se.
> <Gijs> tjr: sgtm, feel free to poke/cc/needinfo me if you have questions or whatever
Reporter

Comment 1

a year ago
> <Gijs> tjr: so the bug confuses me - > <@bz> Only things with direct xpcom access can use this 
> <Gijs> tjr: to me that means "this isn't web-exposed"
> <Gijs> tjr: sorry if I muddied the waters myself here. :(
> <tjr> I understood it as "feedproceessor processes web content and might be passing it in"
> <@bz> Gijs: the SAX bits are not directly web-exposed in that web stuff can't instantiate it
> <@bz> Gijs: The feed code does instantiate it, and may be passing data it gets off the web into it
> <Gijs> tjr: ah, I see. That is accurate. I don't think we can really make the feed processor not pass it web content...
> <Gijs> like, in the sense that if you want to see any of the contents of the feed, that content needs to go into the processor etc.
> <@bz> Gijs: It does mean that there may be things we can prove can't happpen
> <Gijs> mm
> <@bz> (though the API surface here is small enough that may not be much)
> <@bz> e.g. we can probably assume that there are no parseFromString calls while a parseAsync is in flight
> <@bz> (after checking that feedreader does not do that)
> <@bz> whereas if this were web-exposed assuming that would be ... foolhardy.  ;)
> <Gijs> bz: right...
Reporter

Comment 2

a year ago
 Is there anything useful to be gained by slimming down .idl files? In particular I couldn't find any callers of a couple functions, sent it to try, and no compilation errors. https://hg.mozilla.org/try/rev/0839fc73eb8fdd2ccbceceddc863fed7d0a97cfc
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Tom Ritter [:tjr] from comment #2)
>  Is there anything useful to be gained by slimming down .idl files? In
> particular I couldn't find any callers of a couple functions, sent it to
> try, and no compilation errors.
> https://hg.mozilla.org/try/rev/0839fc73eb8fdd2ccbceceddc863fed7d0a97cfc

We've definitely done this in the past with the SAX code, see bug 1416038. You might want to check comm-central as well (not that we have to keep the methods, but we should at least give them a heads up).

Comment 4

a year ago
(In reply to Tom Ritter [:tjr] from comment #2)
>  Is there anything useful to be gained by slimming down .idl files? In
> particular I couldn't find any callers of a couple functions, sent it to
> try, and no compilation errors.
> https://hg.mozilla.org/try/rev/0839fc73eb8fdd2ccbceceddc863fed7d0a97cfc

I think it would be a fine idea to remove stuff that we don't use. I think TB/SM would want to fork this anyhow, because it's liable to getting removed once the feed processing stuff goes away.

I'm not really the best person to comment on how the effort/reward calculus stacks up if we're removing nsSaXXMLReader in the near-ish future (sometime in the next 3-6 months) anyway, to spend effort now on removing unused APIs - I guess binary size might go down, maybe? Perhaps allocations become slightly smaller for instantiating an instance? But overall, I'd expect it to be pretty negligible. I could be wrong though!

(In reply to Tom Ritter [:tjr] from comment #1)
> > <tjr> I understood it as "feedproceessor processes web content and might be passing it in"
> > <@bz> Gijs: the SAX bits are not directly web-exposed in that web stuff can't instantiate it
> > <@bz> Gijs: The feed code does instantiate it, and may be passing data it gets off the web into it
> > <Gijs> tjr: ah, I see. That is accurate. I don't think we can really make the feed processor not pass it web content...
> > <Gijs> like, in the sense that if you want to see any of the contents of the feed, that content needs to go into the processor etc.
> > <@bz> Gijs: It does mean that there may be things we can prove can't happpen
> > <Gijs> mm
> > <@bz> (though the API surface here is small enough that may not be much)
> > <@bz> e.g. we can probably assume that there are no parseFromString calls while a parseAsync is in flight
> > <@bz> (after checking that feedreader does not do that)
> > <@bz> whereas if this were web-exposed assuming that would be ... foolhardy.  ;)
> > <Gijs> bz: right...

Would it be worth poking at this further? I'm not sure what the immediate cause for the filing of this bug was, or if there are overarching security/maintainability/privacy/stability concerns about it or something else.

I hope that's helpful - feel free to ping me on IRC/email or schedule a vidyo discussion if that would be helpful. I've mostly worked with the more frontend-y bits of the feed reader (that live in browser/), but I can page in the feedprocessor stuff where necessary...
Flags: needinfo?(gijskruitbosch+bugs)
Reporter

Updated

a year ago
Depends on: 1448010
Assignee

Comment 5

a year ago
This bug is very confusing. Some comment say that the SAX stuff is used by the feed processing code such that remote content is parsed by the chrome-instantiated parser. (Obviously the case.) The site-compat keyword and the bug title are suggestive of the SAX parser being exposed to Web site JS somehow.

If it is indeed already the case that the SAX parser isn't exposed to Web site JS, can we change the bug title, please? (Otherwise, I'd like to see a demo of Web JS accessing this API.)

Comment 6

a year ago
(In reply to Henri Sivonen (:hsivonen) from comment #5)
> If it is indeed already the case that the SAX parser isn't exposed to Web
> site JS,

I believe so.

> can we change the bug title, please? (Otherwise, I'd like to see a
> demo of Web JS accessing this API.)

Tom? (I'm not sure what to change the title *to*.)
Flags: needinfo?(tom)
Assignee

Updated

a year ago
Priority: -- → P3
Summary: Disable nsSAXXMLReader from Web Content → Remove unused parts of nsSAXXMLReader
Assignee

Comment 7

9 months ago
Given bug 1477667, let's morph this into a full removal depending on that bug.
Depends on: 1477667
Flags: needinfo?(tom)
Summary: Remove unused parts of nsSAXXMLReader → Remove nsSAXXMLReader

Updated

7 months ago
Depends on: 1502954
No longer depends on: 1477667

Updated

7 months ago
Depends on: 1503214

Updated

7 months ago
No longer depends on: 1502954
Assignee

Updated

5 months ago
See Also: → 1514666
Assignee

Updated

5 months ago
See Also: → 1514669
Assignee

Updated

5 months ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Assignee

Comment 10

5 months ago
Not landing right away to give c-c developers a chance to prepare an import-to-c-c patch first.

Comment 11

4 months ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc4350821ea2
Remove the XPIDL SAX interface to the XML parser. r=peterv

Comment 12

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Whiteboard: [adv-main66-]
You need to log in before you can comment on or make changes to this bug.