Configurable gpg executable probably contradicts security policy

[FiloSottile]

The README says

Given full control of the non-native component of the extension, an attacker may be able to list and decrypt .gpg files that can be accessed by the current user, but cannot execute arbitrary code outside of the browser.

however the extension gets to specify the path of the "gpg" binary, its stdin (for the save action), and its final argument (after --output).

It might require a bit of creativity to find a binary that ignores the fixed arguments but still allows arbitrary code execution given these attacker-inputs, but I would not bet against it.

[maximbaz]

Thanks, an interesting insight! We do pass the configured path through a "validate" function, which as of today is focused more on catching typos rather than validating that it actually is a "gpg" binary.

Do you think it would help if we were to edit this function to for example check that stdout of <gpg> --version contains gpg (GnuPG) substring (instead of just checking that this command succeeded), would that be an acceptable solution? Of course if an attacker has write permissions on the file system they can place their own binary and fake this (or probably any other) check, but then there's probably nothing we could do in that case. For information, we also check the $PATH for gpg binary, and overall it's all too common for people to configure the path to gpg binary in the extension, so we can't avoid having that option.

https://github.com/browserpass/browserpass-native/blob/c661efc2ab576f324f76ef33918477b0d2dfc924/helpers/helpers.go#L31-L33

[FiloSottile]

Do you think it would help if we were to edit this function to for example check that stdout of <gpg> --version contains gpg (GnuPG) substring (instead of just checking that this command succeeded), would that be an acceptable solution?

Maybe, I haven't thought it through (and I may not be creative enough, I am not a penetration tester by trade) but it feels like it might be possible to combine the (limited) ability to write files and the control over the argument to cause a binary to print that line and then execute other malicious code.

But also, to check if the stdout of <gpg> --version contains a string you have to execute it, so the attacker still gets to execute arbitrary files.

Personally, if you need to support executing binaries at arbitrary paths across a boundary, I would not claim it as a security boundary, because it feels very very hard to defend successfully.

[erayd]

I agree with @FiloSottile on this. We should document the vulnerability more clearly IMO.

Perhaps it allowing the user to set the gpg binary path only in the JSON config for the password store, and having the next release of the native host parse that bit out, might be a better approach. That file cannot be written by or at the behest of the browser extension, and therefore any compromise of the browser extension would be unable to select an arbitrary binary to execute. This would be more inconvenient for the user, but I suspect only a little bit, and they'd only need to do it once.

@maximbaz Thoughts?

[maximbaz]

I'd be okay with that, and when we do that, perhaps we should show upgrade notification to users who have gpg path configured in localStorage, so that they know what to do. And/or a staged rollout, when we just "deprecate" this in extension, and show a warning in the popup (kinda like "your native host is outdated") that they need to migrate, before we remove this from native host part.

Maybe I'm over-thinking it, but I imagine it's not a small number of people who will need to upgrade (I even know some personally), so it's better to nag people instead of just suddenly break the extension for them.