Hi,
Disclaimer: I am a beginner at JavaScript, sorry for any irrelevant or
mistaken comment below.
Uzair Farooq:
> 1. Our extension script is only injected in https://tails.boum.org,
I'm not sure it's actually the case. My understanding of
scripts/background/background.js is that the extension script is
injected into:
- any existing tab whose URL starts with "
https://tails.boum.org/";
this looks good to me;
- any new or updated tab whose new URL contains the
"
https://tails.boum.org" string somewhere; if I got this right,
this means it's injected in e.g. in:
https://tails.boum.orgx/
https://example.com/https://tails.boum.org
I believe this should be fixed by:
1. looking for the "https://tails.boum.org/" prefix instead of
"https://tails.boum.org"
2. checking that this string is the prefix of the tab's URL, instead
of merely checking it's a substring, i.e. something like
tab.url.indexOf(matches) == 0 (instead of > -1)
What do you think?
> so
> unless there's an iframe on the download page there's no way for any other
> hosts to receive message from our extension. Nevertheless, I've changed the
> target from'*' to 'https://tails.boum.org' to be more safe.
Looks good to me. Note that to review this security-wise, I had to go
read the documentation of postMessage to check how exactly this
argument is used (tl;dr: scheme, hostname, and port must all match for
the event to be dispatched). How about adding a comment about it in
the code?
Wrt. the sending aspect of the extension → tabs message passing, the
extension would happily send messages to any page on our website,
including world-editable ones. I *think* it's not a security problem
but I lack knowledge to analyze this.
> 2. On receiving end we have a check to verify that the source 'window'
> object of the message is same as the 'window' object of the current page
> which essentially means that the site will always reject messages from any
> other page. Nevertheless, I've added an additional check to verify that the
> source of the message is 'https://tails.boum.org'
I'm not sure I follow, due to implicit assumptions about stuff I don't
know (yet). Did I understand correctly that you're implying that when
the message is sent by a contentscript shipped in a WebExtension
(which is the case here), on the receiving end, the source & origin of
the dispatched message are set according to the window that sent the
message? This would make sense to me, and then your reasoning seems to
work just fine, but I'd like to check, so: can you please point me to
the corresponding documentation?
I've verified that in the "origin" property, what matters is protocol
+ host name + port; again, a comment would be welcome (especially
given elsewhere in the same code base, we're using such strings
differently, as a prefix or substring).
> 3. We have checks in place to verify format/data of the messages passed.
I guess you're referring to the fact we're checking the value of
event.data.action. Or did I miss other checks?
In passing, I see that we're assuming the message data (once
de-serialized) has an "action" member. If not, the receiving side will
throw an exception. Thankfully this happens only after we've checked
the identity of the sender so it's not scary. But perhaps you could
check that event.data.action is a thing, and return otherwise, before
using it? I'm not a JS developer so I don't know what the implications
of throwing an exception there vs. returning are.
> Other than that I don't think there's anything else to be worried regarding
> security.
Good to hear. FTR I only looked at the message passing code, so
I can't vouch for anything else.
Once we're done here, I'd like to see the security reasoning that
leads to the "it's safe" conclusion documented somewhere in the
extension Git repo: we want our code to be friendly to auditors, and
mailing list archive are not an obvious place to find such doc.
@sajolida: I volunteer to take care of this if Uzair does not.
I have a few comments that are mostly off-topic here, about issues
I think need to be resolved before we call this work done:
I see two vendorized libraries in scripts/vendor/.
- I could not find any documentation/process to keep them up-to-date,
e.g. in case of security issues, grave functionality bugs, or
compatibility fixes for new browser versions. What's the plan?
I propose you document the plan as part of the release process
in README.
- I see no copyright/licensing info for scripts/vendor/forge.min.js
Can we redistribute it legally, even without this info?
In any case it would be good to document this in our Git repo.
I see no copyright/licensing info for the rest of the code. Please fix
that. We usually use GPL-3+ and a collective copyright such as:
Copyright $YEAR, Tails developers <tails@???>
But you're free to retain your own copyright on the copyrightable code
you wrote, of course :)
If you copied code that was written by Giorgio Maone, then it must be
licensed under a GPLv3-compatible license, and the corresponding
copyright info must be kept.
Cheers!
--
intrigeri
