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 <[email protected]> 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 _______________________________________________ Tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to [email protected].
