Closed Bug 1227867 Opened 9 years ago Closed 1 month ago

Harden add-on security against the issues outlined in the AMO Validator Bypass add-on

Categories

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

Product:
addons.mozilla.org Graveyard
Component:
Add-on Validation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: sebo, Unassigned)

References

(
URL
)

Details

AMO Validator Bypass showed that there are simple ways to leverage the add-on validation.

The validator should be adjusted to recognize these malicious code patterns.

Sebastian
What's the objective for the add-on validator? We all know that static analysis on JS can provably never work unless we drastically restrict the subset of JS, so what's the threat model, exactly?
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #1)
> What's the objective for the add-on validator? We all know that static
> analysis on JS can provably never work unless we drastically restrict the
> subset of JS, so what's the threat model, exactly?

Just to be super clear, there is no "probably".  There is simply no way to detect malicious code like this in a dynamic language like JS through static analysis of the source code.  There was some discussion in <https://groups.google.com/forum/#!topic/mozilla.dev.static-analysis/qTCBKh2bRsE> to that effect a while ago.
Ehsan: I think you misread "provably" as "probably" :-)

Gerv
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #1)
> What's the objective for the add-on validator? We all know that static
> analysis on JS can provably never work unless we drastically restrict the
> subset of JS, so what's the threat model, exactly?

According to https://github.com/dstillman/amo-validator-bypass#running:

- Data exfiltration
- Local process execution
- Remote code execution

As I understand http://danstillman.com/2015/11/23/firefox-extension-scanning-is-security-theater, it actually suggests to remove rules from the validator in order to remove burdens for conscientious developers and the main point of the AMO Validator Bypass extension is to show that auto-validating can't block malware.

Though the rules for detecting malicious code like outlined by AMO Validator Bypass should be improved.

Sebastian
The plan for signing had always been to gradually improve the validator as new threats were discovered, and as our resources permitted. Fortunately we have a team working on this now. They're currently porting the validator to pure JS, to make it easier for the community to contribute patches.

I don't know if we will be able to detect the particular workarounds implemented in this bypass add-on; I'll leave that to the dev team to determine and file individual dependencies.
Jorge, read the static-analysis post. Your own experts are telling you (and the author of the validator) that it is literally not possible to prevent anything via automated scanning. That includes eval(), which means that any arbitrary code can be executed without even bothering to adjust it to pass the scanner.
I think what wasn't mentioned in the thread linked to above that this is very much like solving the halting problem. The halting problem is not a hard problem to solve towards one can make progress -- it is proven to be unsolvable for all systems that are Turing machines (and JS implements a Turing machine). I have always liked this explanation: https://www.youtube.com/watch?v=macM_MtS_w4

Turing was a pretty clever man. Not impossible to prove wrong, but if I were a betting man...
(In reply to Gervase Markham [:gerv] from comment #3)
> Ehsan: I think you misread "provably" as "probably" :-)

Indeed!

(In reply to Jorge Villalobos [:jorgev] from comment #5)
> I don't know if we will be able to detect the particular workarounds
> implemented in this bypass add-on; I'll leave that to the dev team to
> determine and file individual dependencies.

We can definitely add specific checks that would catch the specific patterns used in the bypass add-on.  But my (and David's) point was that it's impossible to extend the validator to catch *all* possible code patterns resulting in a malicious behavior.

For example, there are infinitely many code constructs that can run a local process.  We can detect some (maybe many) of those constructs in the validator, but there is no way for us to detect all of them.  Without that, if the validator catches some code that is trying to do this in an add-on written by a malicious author, the author only needs to massage the code a little bit to obscure what it's trying to do to bypass the validator check.  In a language such as JS, this is effectively trivially simple.  Therefore, attempting to catch specific bad patterns in the validator by analyzing the source code cannot keep out the malicious actor.
Being able to catch every possible bad behavior isn't a requirement for this system to work. If we make it sufficiently difficult for malware creators, and more importantly, if we draw a clear line between normal add-on code and code that is actively trying to bypass our tests, we can more easily detect and block bad actors. Note that we're not doing away with blocklisting and we don't claim to catch every bad add-on before it's signed.
And the validator will helpfully point out the points you need to massage, so unless someone is going to review every single validator attempt (at which point as a malware author I'd just flood the system with spurious attempts) there won't even be a trigger to add new specific checks. The code patterns as mentioned above will effectively have to be found manually, mostly by chance, and added to the pattern detection manually.

I think that what Ehsan is saying is that the pattern you'd have to recognize to mitigate this structurally boils down to "uses object property access", because that's as close as you can actually get in detecting these constructs. This effectively means every single line of JS code anywhere is suspect. Such is the nature of dynamic, Turing-complete languages like Javascript. Which is not to say that the scanner can't give good coding style tips (I enforce coding styles using coffeelint myself and benefit greatly from it), but it can't realistically do more than that.

This is not a matter of gradually improving the validator (or rewriting it in JS) so it catches more and more classes of problems. It's about solving the equivalence/halting problem. The equivalence problem is either NP-hard or proven undecidable (I forgot which). The halting problem is trivially (although brilliantly, Turing is a beast) proven non-solvable.
BTW the way I understood Dan's explanation is that the POC was not intended to show that the validator is structurally flawed -- I think everyone gets that -- but that the only part of the security push that actually accomplishes something is signing + *actual* review. What the POC highlights is that review is effectively optional anyway if you don't want to play ball, so why not make it optional full stop (which is what whitelisting does)?

Whether or not you agree with that last conclusion (I have my suspicions) is a different matter, but I think that the POC and the assessment in this bug makes it pretty clear that malware authors do in fact have free choice whether they wish to be reviewed or not.
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> Being able to catch every possible bad behavior isn't a requirement for this
> system to work. If we make it sufficiently difficult for malware creators,
> and more importantly, if we draw a clear line between normal add-on code and
> code that is actively trying to bypass our tests, we can more easily detect
> and block bad actors. Note that we're not doing away with blocklisting and
> we don't claim to catch every bad add-on before it's signed.

Ehsan Akhgari's comment is relevant here.

> We can definitely add specific checks that would catch the specific
> patterns used in the bypass add-on.  But my (and David's) point was
> that it's impossible to extend the validator to catch *all* possible
> code patterns resulting in a malicious behavior.

I think that last sentence should be articulated. That is not saying, "while it can't be 100% effective, it'll still be effective, just inevitably letting a small number through." It's not just that it can't catch *all* possible code patterns, it's that there will always be an *infinite* number of uncaught patterns, and that it's trivially easy to come up with a new one. This, in my opinion, makes the validator ineffective. It will not make it sufficiently difficult for malware creators. 

The validator would have to keep being updated with new malicious code patterns as they were discovered, and even then there would still always be an infinite number of uncaught malicious patterns. That cat-and-mouse game is basically the same as just banning a specific addon.

For your second point:

> if we draw a clear line between normal add-on code and code that is
> actively trying to bypass our tests, we can more easily detect and
> block bad actors.

The validator won't be able to automatically detect that.

I say that fixing this bug and truly hardening the validator is, unfortunately, impossible.
Yes, just to remove any confusion, the point of the PoC (and the accompanying post) wasn't that the scanner should somehow be updated to catch these examples. The point was that it can't be, without blocking essentially all extensions. I could keep updating the PoC to prove that, but I'd like to think that won't be necessary.

Also, the first example, of stealing sensitive data and sending it elsewhere — presumably a pretty common malware pattern — didn't even require a validator bypass. The extension is just reading an HTTP header and POSTing it. If you tried to block all extensions that accessed HTTP headers, it could just circumvent that as in the other two examples (or get the data from a webpage, or the login manager, or…).

As Emiliano says, under the current system, manual reviews are optional, which means they'll only impact legitimate developers. All arguments stemming from an assumption otherwise are invalid, as is this bug.
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> Being able to catch every possible bad behavior isn't a requirement for this
> system to work. If we make it sufficiently difficult for malware creators,
> and more importantly, if we draw a clear line between normal add-on code and
> code that is actively trying to bypass our tests, we can more easily detect
> and block bad actors.

I don't think there's any evidence that this is in fact practical
with any reasonable amount of effort on the validator, for the reasons
that Ehsan indicated.


> Note that we're not doing away with blocklisting and
> we don't claim to catch every bad add-on before it's signed.
I think Ehsan spoke in terms of (im)possibility rather than (in)practicality. It is in principle impossible to make progress towards a goal that requires an infinite amount of discrete tests; such is the nature of infinity.  "Infinite" above was not a metaphor for "very, very many". There are literally infinitely many undetectable variants, and you can trivially pump out these variants algorithmically. Easy to produce, impossible to detect. 

Again, good code layout standards can really help. It's just that enforcing code layout standards cannot do the job of a security audit. Oh and this is not merely a Turing machine halting problem. There are algorithms that are also principally undecidable for humans, too. 

This is not something you can fix with additional effort. This is not hyperbole. The static analysis people from mozilla also know this.
Not sure if this option has been brought up before, but can we not deal with this threat by adding runtime checks in gecko? Say we have a static analysis validator that checks for accesses to properties X, Y, and Z, and doesn't sign the add-on if it finds them. Furthermore, if it does sign the add-on, it adds a flag to the packaged add-on that says "this add-on doesn't access sensitive properties". In combination with this, we instrument the bindings in gecko for properties X, Y, and Z. The instrumentation would check if the property is being accessed by an add-on that has the flag, and if it is then it knows something is amiss - the add-on must have bypassed the static analysis and is likely malicious. It can then block the entire add-on or just those property accesses, and can report that the add-on needs to be manually reviewed. There would probably be some runtime overhead for the checks but we already have things like CallerIsChrome checks that presumably have roughly the same overhead, so there is precedent for this.
I believe that kats' suggestion is a good (if probably not high-performance) way to simplify the problem a lot. Once we extract e.g. a set of modules and XPCOM interfaces that are used by the add-on, the reviewers can perform a quick review by just looking at this set and checking out whether they make sense with the features expected from the add-on. This will not prevent all malware, far from it, but at least, this should raise the bar without getting too much in the way.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> Not sure if this option has been brought up before, but can we not deal with
> this threat by adding runtime checks in gecko?

Addons can easily evade runtime detection. For example, the malicious payload could be enabled only after a particular date or on a particular version of Firefox. Or the addon could conveniently download malicious code over the internet after passing verification. The possibilities are endless.

I think the only way to battle malicious addons is require addon developers to submit an application for a personal signing key with which they would sign addons. This doesn't prevent malicous addons per se, but at least we will be able revoke the key for a particular developer if they turn out to be malicous. This model would of course require that we verify the identity of new developers in a sufficiently rigorous fashion.
(In reply to Birunthan Mohanathas [:poiru] from comment #18)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> > Not sure if this option has been brought up before, but can we not deal with
> > this threat by adding runtime checks in gecko?
> 
> Addons can easily evade runtime detection. For example, the malicious
> payload could be enabled only after a particular date or on a particular
> version of Firefox. Or the addon could conveniently download malicious code
> over the internet after passing verification. The possibilities are endless.

I'm assuming that all malicious activity involves interaction with core gecko code in some way. It doesn't matter when or how the addon decides to become malicious, the moment it does it has to access some gecko property that it isn't supposed to, and that's when we catch it. The runtime checks I'm proposing aren't a one-time thing, they're run every time the add-on tries to access properties in gecko, forever.

Thinking about this more, what I'm proposing boils down to separating addons into privileged and unprivileged. Unprivileged addons are not allowed access to any APIs that could enable malicious behaviour, and they can also be signed by an automatic validator. Privileged addons can access more APIs, but require human verification during signing. I'm assuming that there is already some support built into Gecko for this sort of thing since we have the notion of privileged/certified apps in Gaia which also have access to special APIs that regular apps don't. I don't know what that machinery looks like or if it can be reused though.
Whenever executing some code, Gecko knows at all times whether that code belongs to an add-on. I believe that we can instrument `Cu.import` and `Cc` without too many difficulties to check from a whitelist. This is XPConnect code, so we don't need to dive in SpiderMonkey to do that.

We'll have holes (e.g. bug 1227283, and most likely with the subscript loader, possibly eval, etc.), but they can be fixed progressively.
@poiru: I think the proposal meant this runtime check would run all the time, like @kats mentioned.

@kats: what you're describing is pretty much the same as "all extensions must pass review", which brings us back to why Dan poked the bear to begin with -- review lead times (and yes, I'm aware that this does not relate to the bug title, but then neither do proposals for run-time checking of API access).
@Emiliano We can bring this down to a very simple review for 99% of reviews, see comment 17. That would already be a big win.
Lots to reply to, sorry for the massive comment (and thank you for the feedback, I don't mean to sound so defensive).

(In reply to Emiliano Heyns from comment #10)
> And the validator will helpfully point out the points you need to massage,
> so unless someone is going to review every single validator attempt (at
> which point as a malware author I'd just flood the system with spurious
> attempts) there won't even be a trigger to add new specific checks.

Historically we've learned about malware from user reports, mostly people who file blocklisting bugs, but also via email an other means. We're not stopping that, and will use those reports to refine the validator so it catches those new patterns, where possible.

> I think that what Ehsan is saying is that the pattern you'd have to
> recognize to mitigate this structurally boils down to "uses object property
> access", because that's as close as you can actually get in detecting these
> constructs.

There aren't many legitimate add-ons that are actively trying to obfuscate their access to the Components object. They generally will pass a static string to get a particular component, so we can flag add-ons that try to pass dynamically generated strings to it. This is not hard to bypass using eval and other string evaluation, but that's something we also flag. There will be other ways to do the same, and we'll deal with them case-by-case.

> This is not a matter of gradually improving the validator (or rewriting it
> in JS) so it catches more and more classes of problems.

Yes, it's a cat-and-mouse game, and it's not 100% solvable. We knew that very well when we started this.

(In reply to Emiliano Heyns from comment #11)
> BTW the way I understood Dan's explanation is that the POC was not intended
> to show that the validator is structurally flawed -- I think everyone gets
> that -- but that the only part of the security push that actually
> accomplishes something is signing + *actual* review. What the POC highlights
> is that review is effectively optional anyway if you don't want to play
> ball, so why not make it optional full stop (which is what whitelisting
> does)?

I do believe that adequate validation should filter out the majority of malware. I have been in charge of the blocklist for many years and I know the patterns they follow are very simple and predictable. They will adjust, for sure, but they will always go with what's simplest because their goal is to get a big install base in a very short time. I also believe that if we make this hard enough for them, they'll find other easier ways to try to attack users.

On the other hand, I admit the validator is not even close to doing this. There have been many changes to the developers working on it for the past few months that have basically stalled development, and we're currently waiting on the JS implementation of the validator before we can introduce bigger changes to the way it works. At the moment it's essentially the same validation we use for AMO, which was mostly meant to warn reviewers about things they should look at more carefully.

(In reply to tucker.mcknight from comment #12)
> For your second point:
> 
> > if we draw a clear line between normal add-on code and code that is
> > actively trying to bypass our tests, we can more easily detect and
> > block bad actors.
> 
> The validator won't be able to automatically detect that.

No, which is why there's a human component to this. We didn't intend this to be a complete filter for malware, but also a way to more easily track it down and analyze it if reported. We'll have access to the malicious XPIs, figure out if there are other similar ones, and determine if there's anything we can automatically detect to stop future submissions following the same patterns. Having non-AMO add-ons blocked by review was a requirement that was introduced later on, and what's caused the most friction with developers. It is being reconsidered.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> Not sure if this option has been brought up before, but can we not deal with
> this threat by adding runtime checks in gecko?

This might be possible for WebExtensions, since the API will be more limited and sandboxed. XUL extensions have too much power to be controllable at runtime.

Related to this, we're also looking into adding automated feature testing, where the extension would run in an actual Firefox instance, which would be able to detect certain things that the validator can't, like POSTing private data as exemplified in the bypass PoC. This, of course, has its own flaws and limitations, but would be an additional layer of protection.
(In reply to Jorge Villalobos [:jorgev] from comment #23)
> This might be possible for WebExtensions, since the API will be more limited
> and sandboxed. XUL extensions have too much power to be controllable at
> runtime.

Definitely easier with WebExtensions, but I'm not sure it's impossible for XPCOM-based extensions. Can you think of cases which are not covered by comment 20?
Flags: needinfo?(jorge)
As far as detecting access to certain XPCOM APIs and maybe bad uses to those APIs, I think that's a reasonable approach. That wouldn't cover other cases of bad behavior like add-ons sending private data to remote servers or injecting remote scripts into content, which are more common concerns in malware, but I agree it would be an improvement.

Whether this is worth the investment given the move to WebExtensions, I don't know. I think we should talk to Kev about this, to figure out if we should pursue it.
Flags: needinfo?(jorge)
(In reply to Jorge Villalobos [:jorgev] from comment #23)
> There aren't many legitimate add-ons that are actively trying to obfuscate
> their access to the Components object. They generally will pass a static
> string to get a particular component, so we can flag add-ons that try to
> pass dynamically generated strings to it.

No, you can't. Components is a property of ChromeWindow, which can be accessed from any element.

document.createElement('label').ownerDocument.defaultView.Components

Now imagine those are all dynamic properties.

> This is not hard to bypass using
> eval and other string evaluation, but that's something we also flag. There
> will be other ways to do the same, and we'll deal with them case-by-case.

You can't flag eval.

> I do believe that adequate validation should filter out the majority of
> malware. I have been in charge of the blocklist for many years and I know
> the patterns they follow are very simple and predictable. They will adjust,
> for sure, but they will always go with what's simplest because their goal is
> to get a big install base in a very short time. I also believe that if we
> make this hard enough for them, they'll find other easier ways to try to
> attack users.

You cannot make this "hard". That was the point of my PoC, and that's what everyone on this bug has been trying to communicate. It will always be trivial. It's not a "cat-and-mouse game" — you literally cannot put in place rules to block these things.

I think your confusion here is partly my fault, actually — the actual patterns in my PoC were meant to be somewhat humorous, but they shouldn't be seen as representative. The point is that in JS you can access a property of an object with a string, and that string can come from anywhere, and it is provably impossible to determine what that string will be.

As for runtime checks of various kinds, I've assumed that doing those in XPCOM wouldn't be deemed worth the effort with WebExtensions on the horizon, as Jorge suggests. And as he says, there's plenty of malware you can create without XPCOM or with potentially innocuous interfaces, so I don't think it's as simple as separating extensions into privileged and not privileged. But yes, one way or another, documenting allowed capabilities in the manifest is the only way to block these things.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #1)
> What's the objective for the add-on validator?

A primary objective of the validator is to check that the add-on is correct and well formed. For example: to check that it has files in the correct place, data in fields is within normal bounds etc. As another example: it can let you know if you are using an API that is now deprecated.

The validator has built these rules up by scanning all the add-ons coming into AMO over the last 5+ years.
Splendid! As Zotero conforms to these rules, the fact it doesn't pass for automated signing is thus a bug to be fixed in the scanner, yeah?
Oh and to be specific the validator hasn't built up these rules, it merely enforces the specific instances it has been told about. The scanner could have been running forever and it wouldn't have found a single new instance even of a known threat. Which was Dan's point.
Wow, I didn't mean to set off an avalanche of comments regarding add-on validation and signing. I'd expect this to happen in Dan's post to the mailing list[1].
This bug was really just meant to add detection for the "malicious code" related to Dan's extension.
I'd be absolutely fine if this report was closed as WONTFIX if you believe that other steps need to be taken (like the solution kats pointed out in comment 16) to improve the automatic validation of malicious extensions.

Of course you are not fully protected against malicious software without having each uploaded version of every add-on tested manually (and even with manual testing malicious code may slip through). Though as this is impossible in regard of human resources, an automated solution must help the reviewers identify possible security risks.

Relating to Dan's first suggestion:
Having a software signed gives a user the feeling that it is secure to use it. Though if that software is only validated automatically, it is impossible to say that it is free of bad code. So auto-signing it conveys a wrong impression of security.
Therefore signing should only happen when a version of an add-on was manually tested.
(Though that topic is very basic and I hope it was already discussed before auto-signing all add-ons on AMO.)

Sebastian

[1] https://mail.mozilla.org/pipermail/firefox-dev/2015-November/003554.html
(In reply to Sebastian Zartner [:sebo] from comment #30)
> Relating to Dan's first suggestion:
> Having a software signed gives a user the feeling that it is secure to use
> it. Though if that software is only validated automatically, it is
> impossible to say that it is free of bad code. So auto-signing it conveys a
> wrong impression of security.
> Therefore signing should only happen when a version of an add-on was
> manually tested.

OT, but just to be clear, this wasn't my argument. Signing itself isn't the problem — it's claiming that automated scanning can identify malicious code in any real way, and sending legitimate extensions to manual review because of it.

This bug can be closed as INVALID (not WONTFIX), since it's not possible to block the behaviors in the PoC in the scanner itself.
(In reply to Dan Stillman from comment #31)
> not possible to block the behaviors in the PoC in the scanner itself.
Well *technically*, if the scanner would simply reject anything that parses into an AST, it would in fact block the behaviors in the PoC. Getting past that *would* raise the bar immensely.

(In reply to Sebastian Zartner [:sebo] from comment #30)
> I'd be absolutely fine if this report was closed as WONTFIX if you believe
> that other steps need to be taken (like the solution kats pointed out in
> comment 16) to improve the automatic validation of malicious extensions.

Comment #16 in fact proposes run-time restrictions rather than code validation. It achieves something differently altogether. Something that does not involve solving a logical paradox, which is a nice side benefit I would say.
(In reply to Emiliano Heyns from comment #28)
> Splendid! As Zotero conforms to these rules, the fact it doesn't pass for
> automated signing is thus a bug to be fixed in the scanner, yeah?

Yes, we have issues with some add-ons that time out. That's probably addressed by a seperate bug and we think we can get some quick workarounds in place. I will also point out that the rewrite into Javascript is much faster (at the moment) and I'm not anticipating similar problems.
(In reply to Dan Stillman from comment #31)
> (In reply to Sebastian Zartner [:sebo] from comment #30)
> > Relating to Dan's first suggestion:
> > Having a software signed gives a user the feeling that it is secure to use
> > it. Though if that software is only validated automatically, it is
> > impossible to say that it is free of bad code. So auto-signing it conveys a
> > wrong impression of security.
> > Therefore signing should only happen when a version of an add-on was
> > manually tested.
> 
> OT, but just to be clear, this wasn't my argument. Signing itself isn't the
> problem — it's claiming that automated scanning can identify malicious code
> in any real way, and sending legitimate extensions to manual review because
> of it.

FWIW, signing is not typically used to mean that the signed code is secure and/or safe.  It's used to prove that the code comes from the source indicated in the signature and has not been tampered with after being signed.

There is no way to make sure that the code is safe before signing it.  I have already stated why it's impossible to do by analyzing the source code using software.  Analysis by humans is ineffective in other ways assuming the adversarial author is trying to mislead the reviewer.  But obviously a trained individual, given enough time to analyze the code in question, can raise the bar, but we can never be sure the code is secure before signing it.

About the runtime detection of security policy violations done by add-ons, simple ideas such as what kats suggested won't work because aren't really effective because a) even though we sometimes know we're executing code on behalf of an add-on, we can't assume that we know that for all cases, and b) we can't just restrict access to a property, for example, since that may take away useful functionality.  In order to for example prevent against an add-on uploading a private piece of data, we'd need to do a full-blown taint analysis which is impractical with an API surface area the size of ours.  And the solution would most likely turn out to be too expensive anyway.

I'll stop now since we're already way beyond off topic in this bug.  :-)
Dan and I agree that signing does these things and that they constitute a benefit.

We also agree that the scanner cannot do to any significant extent what it purports to do given the fact that it has the power to OK signing (which implies, in the current proposal, sign-off, as it is offered as an alternative to manual review), and that hardening -- in the sense of recognizing the pattern behind the bypass rather than the specific way the bypass puts the patterns in code -- against the problems the PoC highlights is simply not possible. Anything it does catch would be because the extension dev blindly copy-pasted from the PoC, and the addition or change of a character or two (conveniently pointed out by the scanner exactly where) would present a wholly new case the scanner which it would be structurally unable to catch until explicitly told about. But we seem to be a minority in agreeing on this.

We also agree that "given enough time" that the bar can indeed be raised using manual review, but the "given enough time" is *exactly* the point of contention.

I am not in any way opposed to run-time detection. Whether or not it is practical to do so I can't say as it's far outside my area of expertise.
(In reply to :Ehsan Akhgari from comment #34)
> About the runtime detection of security policy violations done by add-ons,
> simple ideas such as what kats suggested won't work because aren't really
> effective because a) even though we sometimes know we're executing code on
> behalf of an add-on, we can't assume that we know that for all cases,

I suspect that the number of holes is finite.

I realize that there are very tricky cases, such as anything that uses XML/HTML to magically turn a string into code (`foo.addEventListener("bar", "some code")`). I believe that we should strive to plug these holes, because if I understand correctly what I've read from WebExtensions, we're going to be stuck with at least HTML in add-ons for a long time.


> and b)
> we can't just restrict access to a property, for example, since that may
> take away useful functionality. In order to for example prevent against an
> add-on uploading a private piece of data, we'd need to do a full-blown taint
> analysis which is impractical with an API surface area the size of ours. 
> And the solution would most likely turn out to be too expensive anyway.

This would indeed be asking too much from this approach. However, what we can do is
1/ ship a manifest with an add-on (listing all the XPCOM classes and the jsm that the add-on can import – this manifest can often be extracted by static analysis);
2/ review the manifest against the expected feature set of the add-on, use it to determine whether the add-on needs a manual review;
3/ use runtime detection to prevent violation of the manifest;
4/ report attempted violations through Telemetry, which may indicate either a faulty static analysis/manifest, a bug in the add-on or an attempted hack.

I believe that this would both raise the security bar and decrease the amount of time spent reviewing.

> I'll stop now since we're already way beyond off topic in this bug.  :-)

Not anymore :)
Summary: Harden the validator against the issues outlined in the AMO Validator Bypass add-on → Harden add-on security against the issues outlined in the AMO Validator Bypass add-on
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #36)
> 
> I suspect that the number of holes is finite.

They might be. The problem however is that the three Dan highlighted are epxloitable in an infinite amount of ways.

> 3/ use runtime detection to prevent violation of the manifest;
>
> I believe that this would both raise the security bar and decrease the
> amount of time spent reviewing.

Finally, a voice of reason.
(In reply to Emiliano Heyns from comment #37)
> (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment
> #36)
> > 
> > I suspect that the number of holes is finite.
> 
> They might be. The problem however is that the three Dan highlighted are
> epxloitable in an infinite amount of ways.

We're not talking about the same set of holes. I'm talking about the ones mentioned by ehsan « a) even though we sometimes know we're executing code on behalf of an add-on, we can't assume that we know that for all cases »
Ah, Ok,that makes sense.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #36)
> (In reply to :Ehsan Akhgari from comment #34)
> > About the runtime detection of security policy violations done by add-ons,
> > simple ideas such as what kats suggested won't work because aren't really
> > effective because a) even though we sometimes know we're executing code on
> > behalf of an add-on, we can't assume that we know that for all cases,
> 
> I suspect that the number of holes is finite.

Yes, but the problem is that as long as there is a single one open, malicious actors can use that and bypass all of our checks.

Note that I'm assuming that we're looking for a way to make it possible to automatically sign an extension and let it run on our users' Firefox profiles and guarantee that it is not malicious (which I am not convinced is a reasonable ask, at least for the current set of add-ons.)  If the goals are different, the runtime detection solution may be a suitable one, but it depends on what we're trying to defend against.

> I realize that there are very tricky cases, such as anything that uses
> XML/HTML to magically turn a string into code (`foo.addEventListener("bar",
> "some code")`). I believe that we should strive to plug these holes, because
> if I understand correctly what I've read from WebExtensions, we're going to
> be stuck with at least HTML in add-ons for a long time.

The mere fact that you can inject code is not as problematic as what level of privileges the injected code will have.  If WebExtension's security model is loading HTML coming from an add-on in a privileged context, then this won't help.  But if they run with normal content privileges, then things should be fine.  I am not familiar with the security model of WebExtensions in any meaningful level of detail.

> > and b)
> > we can't just restrict access to a property, for example, since that may
> > take away useful functionality. In order to for example prevent against an
> > add-on uploading a private piece of data, we'd need to do a full-blown taint
> > analysis which is impractical with an API surface area the size of ours. 
> > And the solution would most likely turn out to be too expensive anyway.
> 
> This would indeed be asking too much from this approach. However, what we
> can do is
> 1/ ship a manifest with an add-on (listing all the XPCOM classes and the jsm
> that the add-on can import – this manifest can often be extracted by static
> analysis);
> 2/ review the manifest against the expected feature set of the add-on, use
> it to determine whether the add-on needs a manual review;

We can't know what set of XPCOM components an add-on uses a priori, similar to how we cannot know whether it uses some blacklisted APIs.

> 3/ use runtime detection to prevent violation of the manifest;
> 4/ report attempted violations through Telemetry, which may indicate either
> a faulty static analysis/manifest, a bug in the add-on or an attempted hack.
> 
> I believe that this would both raise the security bar and decrease the
> amount of time spent reviewing.

Again it depends on what we're trying to defend against.  But note that the usefulness of any runtime checks for assisting the manual reviewer should not be exaggerated: it is way too easy for an extension to "disable" its attack code before it has been signed, so the extension may "behave" while it's being reviewed.
(In reply to :Ehsan Akhgari from comment #40)
Let me answer the following first, to avoid confusions:

> But note that the
> usefulness of any runtime checks for assisting the manual reviewer should
> not be exaggerated: it is way too easy for an extension to "disable" its
> attack code before it has been signed, so the extension may "behave" while
> it's being reviewed.

This is not what I suggest. I suggest that the runtime checks take place on the end user's computer.

> Yes, but the problem is that as long as there is a single one open,
> malicious actors can use that and bypass all of our checks.
> 
> Note that I'm assuming that we're looking for a way to make it possible to
> automatically sign an extension and let it run on our users' Firefox
> profiles and guarantee that it is not malicious (which I am not convinced is
> a reasonable ask, at least for the current set of add-ons.)  If the goals
> are different, the runtime detection solution may be a suitable one, but it
> depends on what we're trying to defend against.

Not exactly. I want to guarantee that the behavior of the add-on doesn't exceed a security signature specified in a manifest. For the time being, I define a security signature as a list of XPCOM classes referenced directly, jsm modules referenced directly and Jetpack modules referenced directly.

The runtime detection aims to defend against a single threat: add-ons that obfuscate themselves so as to make static analysis miss API entry points.

As you mention, as long as there is a single hole open, a malicious actor can escalate and grab all privileges – again, I define "hole" as "place where code injected by an add-on can pretend it was injected by someone else". However, even if there are holes, we have raised the bar from "let's use simple string concatenation to evade static analysis" to "I need to find a security hole in the Firefox APIs to which I am exposed". Unless our holes are absolutely trivial to find and exploit, from then on, it's the usual security arms race, which is infinitely better than both what we have at the moment and what is proposed around signing.

Also, there may be way to get static analysis on our C++ code to aid us get rid of these holes, but let's not venture into this just yet.

> The mere fact that you can inject code is not as problematic as what level
> of privileges the injected code will have.  If WebExtension's security model
> is loading HTML coming from an add-on in a privileged context, then this
> won't help.  But if they run with normal content privileges, then things
> should be fine.  I am not familiar with the security model of WebExtensions
> in any meaningful level of detail.

I realize that I wasn't clear. The problem I was mentioning is that while scripts have an add-on ID, I seem to remember that strings don't, so any silent promotion of a string into code may introduce a hole. While I'm almost sure that `eval` and `new Function()` handle this correctly, I'm almost sure that the DOM doesn't. In other words, I believe that we agree.

Now, I am also unclear about WebExtensions security.

> > This would indeed be asking too much from this approach. However, what we
> > can do is
> > 1/ ship a manifest with an add-on (listing all the XPCOM classes and the jsm
> > that the add-on can import – this manifest can often be extracted by static
> > analysis);
> > 2/ review the manifest against the expected feature set of the add-on, use
> > it to determine whether the add-on needs a manual review;
> 
> We can't know what set of XPCOM components an add-on uses a priori, similar
> to how we cannot know whether it uses some blacklisted APIs.

Indeed, we can't have a provably correct list. This is why I propose we replace static analysis (which is broken for this purpose) by a hybrid static/dynamic analysis. The static phase extracts a security signature as a manifest (see above) and the dynamic phase ensures that the signature is not exceeded.
Flags: needinfo?(ehsan)
If we're going this route, why not have the extension author declare a manifest explicitly? What's the benefit from extracting it by code analysis?
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #41)
> > But note that the
> > usefulness of any runtime checks for assisting the manual reviewer should
> > not be exaggerated: it is way too easy for an extension to "disable" its
> > attack code before it has been signed, so the extension may "behave" while
> > it's being reviewed.
> 
> This is not what I suggest. I suggest that the runtime checks take place on
> the end user's computer.

Cool.

> > Yes, but the problem is that as long as there is a single one open,
> > malicious actors can use that and bypass all of our checks.
> > 
> > Note that I'm assuming that we're looking for a way to make it possible to
> > automatically sign an extension and let it run on our users' Firefox
> > profiles and guarantee that it is not malicious (which I am not convinced is
> > a reasonable ask, at least for the current set of add-ons.)  If the goals
> > are different, the runtime detection solution may be a suitable one, but it
> > depends on what we're trying to defend against.
> 
> Not exactly. I want to guarantee that the behavior of the add-on doesn't
> exceed a security signature specified in a manifest. For the time being, I
> define a security signature as a list of XPCOM classes referenced directly,
> jsm modules referenced directly and Jetpack modules referenced directly.

Hmm, what threat model are you addressing?  I'm asking because I can't immediately map the list of XPCOM classes to a useful security property.  For example, let's say the add-on uses nsIProcess.  Using that to launch PossiblyEvil.exe is very different than using it to launch Notepad, and for some add-ons (such as a custom view source add-on) the latter should be considered OK.  Merely gating things on the XPCOM component uses doesn't let us differentiate the two.

The answer to the rest of your comment really depends on the problem we're solving...

> Also, there may be way to get static analysis on our C++ code to aid us get
> rid of these holes, but let's not venture into this just yet.

I don't think that's possible.  The types of holes I'm worried about are more about APIs dealing with each other in surprising ways, not simple mistakes a C++ static checker can catch.

> > The mere fact that you can inject code is not as problematic as what level
> > of privileges the injected code will have.  If WebExtension's security model
> > is loading HTML coming from an add-on in a privileged context, then this
> > won't help.  But if they run with normal content privileges, then things
> > should be fine.  I am not familiar with the security model of WebExtensions
> > in any meaningful level of detail.
> 
> I realize that I wasn't clear. The problem I was mentioning is that while
> scripts have an add-on ID, I seem to remember that strings don't, so any
> silent promotion of a string into code may introduce a hole. While I'm
> almost sure that `eval` and `new Function()` handle this correctly, I'm
> almost sure that the DOM doesn't. In other words, I believe that we agree.

Ah, yes, now I understand.  Yeah such strings are one example of the holes we'd need to plug.

> > > This would indeed be asking too much from this approach. However, what we
> > > can do is
> > > 1/ ship a manifest with an add-on (listing all the XPCOM classes and the jsm
> > > that the add-on can import – this manifest can often be extracted by static
> > > analysis);
> > > 2/ review the manifest against the expected feature set of the add-on, use
> > > it to determine whether the add-on needs a manual review;
> > 
> > We can't know what set of XPCOM components an add-on uses a priori, similar
> > to how we cannot know whether it uses some blacklisted APIs.
> 
> Indeed, we can't have a provably correct list. This is why I propose we
> replace static analysis (which is broken for this purpose) by a hybrid
> static/dynamic analysis. The static phase extracts a security signature as a
> manifest (see above) and the dynamic phase ensures that the signature is not
> exceeded.

See above.  I'm unsure how that will help...
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #43)
> Hmm, what threat model are you addressing?  I'm asking because I can't
> immediately map the list of XPCOM classes to a useful security property. 
> For example, let's say the add-on uses nsIProcess.  Using that to launch
> PossiblyEvil.exe is very different than using it to launch Notepad, and for
> some add-ons (such as a custom view source add-on) the latter should be
> considered OK.  Merely gating things on the XPCOM component uses doesn't let
> us differentiate the two.

So something like nsIProcess which could definitely be used for malicious purposes should be flagged using static analysis in the validator, so that an add-on using nsIProcess would require manual review and would not get automatically signed. If an add-on is using nsIProcess using obfuscation in such a way so as to defeat the validator, then it would get caught at runtime, because nsIProcess wouldn't be in the manifest generated by the validator, and the runtime checks would say "hey you weren't cleared to use this API, even if it's for something innocuous like notepad.exe". Does that make sense?
(In reply to :Ehsan Akhgari from comment #43)
> Hmm, what threat model are you addressing?  I'm asking because I can't
> immediately map the list of XPCOM classes to a useful security property. 
> For example, let's say the add-on uses nsIProcess.  Using that to launch
> PossiblyEvil.exe is very different than using it to launch Notepad, and for
> some add-ons (such as a custom view source add-on) the latter should be
> considered OK.  Merely gating things on the XPCOM component uses doesn't let
> us differentiate the two.
> 
> The answer to the rest of your comment really depends on the problem we're
> solving...

I realize that my proposal doesn't come with an intuitive security model. What it does is reduce an unsolvable problem into several smaller issues:
1/ the hybrid static/dynamic analysis;
2/ the list of suspicious classes/modules that flag an add-on for manual review;
3/ the review itself (it may be possible to do only partial reviews, e.g. only the uses of nsIProcess).

In my view, the threat model is part of 2/ and 3/.

I realize that the impedance of XPCOM/Modules somewhat differ from that of security, but we are trying to find a reasonable solution. Also, in an ideal world, we should be able to progressively evolve APIs to wrap some suspicious entry points behind known-to-be-safe entry points. This probably doesn't solve the Notepad issue, of course.

> > Also, there may be way to get static analysis on our C++ code to aid us get
> > rid of these holes, but let's not venture into this just yet.
> 
> I don't think that's possible.  The types of holes I'm worried about are
> more about APIs dealing with each other in surprising ways, not simple
> mistakes a C++ static checker can catch.

That's possible. Again, at that stage, it's an arms race. It is my understanding that WebExtensions will reduce the perimeter.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #45)
> 
> I realize that my proposal doesn't come with an intuitive security model.
> What it does is reduce an unsolvable problem into several smaller issues:
> 1/ the hybrid static/dynamic analysis;

I honestly don't see the point of the static part here. Just let the extension author declare it wants to use that API. If you don't, you don't get to use it at runtime.

> 2/ the list of suspicious classes/modules that flag an add-on for manual
> review;

Which can just be taken from the manifest the dev submits.

> 3/ the review itself (it may be possible to do only partial reviews, e.g.
> only the uses of nsIProcess).

Which can be rather trivially hidden.
(In reply to Emiliano Heyns from comment #46)
> I honestly don't see the point of the static part here. Just let the
> extension author declare it wants to use that API. If you don't, you don't
> get to use it at runtime.

WFM, too. I just wanted to minimize the impact on add-on authors.

> > 2/ the list of suspicious classes/modules that flag an add-on for manual
> > review;
> 
> Which can just be taken from the manifest the dev submits.

This is a list used by reviewers, it's not part of a specific add-on. Example: nsIProcess is suspicious, nsISimpleEnumerator isn't.

> > 3/ the review itself (it may be possible to do only partial reviews, e.g.
> > only the uses of nsIProcess).
> 
> Which can be rather trivially hidden.

Well, not from the reviewers.
I don't mean the list which is considered should  be taken from the manifest (duh) but that the list established by amo can just be matched against that manifest.

If the dev doesn't write the manifest it may bite them at runtime in any case, and debugging would be a pain. Best not to mask this process imo. If you want to help the dev, give them a tool that scans and creates such a manifest as starter code, but have the manifest be explicit. 

And yes, trivially hidden from the reviewer for any extension of any significant size. The replace() calls may stand out but that's because no effort has been spent in trying to hide them in a few thousand lines of code. The proof of concept is deliberately unsubtle.
> And yes, trivially hidden from the reviewer for any extension of any significant size. The replace() calls may stand out but that's because no effort has been spent in trying to hide them in a few thousand lines of code. The proof of concept is deliberately unsubtle.

Ah, I had completely misunderstood what you had written. Yes, fair enough, maybe partial reviews are not possible.
And I could have been a little more polite about it, sorry.
Looks like there was already some discussion on the runtime checking of what add-ons are doing, according to what bholley said at https://groups.google.com/d/msg/mozilla.dev.platform/AGW3-zSBjl8/m3JJO2CECgAJ.
See Also: → 1227464
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44)
> (In reply to :Ehsan Akhgari from comment #43)
> > Hmm, what threat model are you addressing?  I'm asking because I can't
> > immediately map the list of XPCOM classes to a useful security property. 
> > For example, let's say the add-on uses nsIProcess.  Using that to launch
> > PossiblyEvil.exe is very different than using it to launch Notepad, and for
> > some add-ons (such as a custom view source add-on) the latter should be
> > considered OK.  Merely gating things on the XPCOM component uses doesn't let
> > us differentiate the two.
> 
> So something like nsIProcess which could definitely be used for malicious
> purposes should be flagged using static analysis in the validator, so that
> an add-on using nsIProcess would require manual review and would not get
> automatically signed. If an add-on is using nsIProcess using obfuscation in
> such a way so as to defeat the validator, then it would get caught at
> runtime, because nsIProcess wouldn't be in the manifest generated by the
> validator, and the runtime checks would say "hey you weren't cleared to use
> this API, even if it's for something innocuous like notepad.exe". Does that
> make sense?

OK, yeah this makes sense.  But the usefulness of the idea depends on a) how well we can detect when some code is being run on behalf of an add-on, and b) how well we can come up with a list of "potentially dangerous" APIs.  I'm not sure how hard (a) is, but (b) is definitely doable even though it's a lot of work.
Product: addons.mozilla.org → addons.mozilla.org Graveyard
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.