Hi,
(Reordered to make the discussion easier for me to follow than
top-posted replies quoting text that's already quoted below.)
Uzair Farooq:
> On Sat, Nov 25, 2017 at 6:44 PM, intrigeri <intrigeri@???> wrote:
>> Uzair Farooq:
> 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.
Great, thanks. But the other check (that you've partly fixed) will
still allow e.g. "
https://tails.boum.orgx". Is it on purpose or did you
miss this part of my review?
(I understand this check is not relied upon to enforce security, but
as long as we keep it, presumably as part of a security in depth
approach, it makes more sense to me if it accurately produces the
effect you intend it to have.)
>> 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.
Thanks!
>> 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.
I was referring to usage of postMessage.
>> > 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?
> 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
I had read all already that but it was not clear to me how this
applies to WebExtensions: most of the text on that page seems to apply
to regular site content scripts, and the only part that's specifically
about extensions seems to be outdated and to only apply to old-style
add-ons, hence my question :)
> You can read more about content scripts here
> https://developer.chrome.com/extensions/content_scripts
Perfect, this does answer my question, especially once I've followed
the link to
https://developer.chrome.com/extensions/messaging!
>> 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.
OK!
>> 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'll wait for sajolida to give me instructions on this one.
>> 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.
Ping?
>> - 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.
> Also added licensing info to the forge info.
Thanks, but the copyright info is still missing.
>> 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.
Thanks, looks good to me, but the license info is still missing.
>> 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.
tails-download-and-verify/package.json says:
"author": "Giorgio Maone",
"license": "GPLv3"
So the licensing is crystal clear. From the author info I think we can
safely infer copyright 2015-2016 by Giorgio Maone.
Cheers,
--
intrigeri
