Hey,
1. We have specified permission for
https://tails.boum.org in
manifest.json. So, even if a site passes the check, the script still won't
be injected if haven't specified it in manifest.json. I've modified the
check anyway.
> 2. 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?
I've added comments.
> 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.
I'm not sure what you are referring to by 'tabs message passing'. If you're
talking about chrome extenion tabs.sendMessage api, that api can only be
used to communicate within the extension, you can't communicate with web
pages using that api.
> 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?
Yeah, that's correct. The chrome extension content script are considered as
normal site scripts but have access to some additional apis. If we use
postMessage api from contentScript, it's just like we send a message from
the website itself. So, it'll set the source to the window of the object of
the site. Please see the source property in postMessage api
https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage
You can read more about content scripts here
https://developer.chrome.com/extensions/content_scripts
> 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.
Yeah, it'll throw exception and the exception won't have any other
implication other than throwing an error in console. I modified the check a
while back to check for the data property too.
> 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 :)
I've added copy right notices to the files. Also added licensing info to
the forge info. The source/version is already there in the forge lib.
> 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.
There's a small part of code that was copied from old extension but I don't
see any licensing or copyright notice in the original repo.
On Sat, Nov 25, 2017 at 6:44 PM, intrigeri <intrigeri@???> wrote:
> 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
>
