Re: [Tails-dev] Review of verification-extension

Delete this message

Reply to this message
Autore: Uzair Farooq
Data:  
To: u
CC: The Tails public development discussion list
Oggetto: Re: [Tails-dev] Review of verification-extension
>- contentscript/verify.js:
> - fetchConf(): please wrap regexps.


Done.

>- contentscript/conf.js:
> - change the descriptor file to
>https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml


Done

> - are we sure that no other URL could be injected here: ajaxData.url=
> this.conf.descriptor; ?


Yeah, the url is hard coded in config, it can't be changed.

>- What kind of comments does this remove:
>    44 data = data.replace(/^[^'"]*#.*/gm, ''); // remove most comments
>    I don't see any in
>https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml


We copied it from old extension:
https://git-tails.immerda.ch/ma1/download-and-verify-extension/tree/tails-download-and-verify/lib/df.js

> - Can we not verify automatically once the download is finished?


As far as I know, chrome does not allow you to read local files. Someone
has mentioned this workaround
https://stackoverflow.com/questions/41767585/chrome-extension-to-access-content-of-downloaded-files
but I'm not sure if it'll work, we'll have to try.

>- origin of files:
> vendor/forge.js: please add a URL of origin for this script as well as a

version number so that we can update it in the future.

Done.


> - Please use double quotes instead of single quotes.


Done.

> - convert tabs to spaces


Done.

>  - fart operator =>
>    -  example: this.fetchConf().done()=>{
>       JSLint requires the parens around the parameters, and forbids a { left

brace after the => fart to avoid syntactic ambiguity. See:
http://www.jslint.com/help.html#function

Skipping the left brace is only allowed for one line functions.

> - Consider using strict-mode:
>https://developer.mozilla.org/en-US/docs/Web/JavaScript/

Reference/Strict_mode
> We want this code to be forwards compatible as much as possible as well

as as secure as possible.

Done.


> - Whishlist: please document how to test the extension locally in a
>README file.
> - Exclude this README file from the resulting XPI.
> - See as example the HACKING file in tails
>tails@???:download-and-verify-extension


Added a README file


On Mon, Nov 13, 2017 at 11:18 PM, u <u@???> wrote:

> Dear Uzair,
>
> so here is a more complete review of the extension.
>
> As said, I think there are two urgent matters:
>
> - manifest.json:
>   -  "permissions": [
>          "http://*/*",
>      -> please deactivate HTTP. We only want to download over HTTPS.

>
> - contentscript/verify.js:
> - fetchConf(): please wrap regexps.
>
> And here are some other points I realized. If you think that any of
> these points are not applicable, please don't hesitate to comment. I'm
> not an expert in webextensions.
>
> - contentscript/conf.js:
> - change the descriptor file to
> https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml (I think
> sajolida created a ticket on our bugtracker already. It's not urgent,
> because the other files currently contains the same contents.)
>
> - contentscript/verify.js:
>   - are we sure that no other URL could be injected here: ajaxData.url=
> this.conf.descriptor; ?
>     - if not let's try to at least verify that the URL starts with
>       https:// and comes from tails.boum.org
>   - What kind of comments does this remove:
>     44 data = data.replace(/^[^'"]*#.*/gm, ''); // remove most comments
>     I don't see any in
> https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml

>
>   - setVerifyListener(){
>         let self = this;
>         this.$(this.document).on("change", this.conf.verifySelector, (e)
> => {
>         self.calculateHash(e.target);
>         });
>     }
>     -> So here we assume that the person clicks nicely on our button to
> verify and that nobody will interfere.
>        - Can we not verify automatically once the download is finished?
>        - Also, can we have a listener for the hash in the URL?
>          For example, if I closed the window but now I want to come back
> and just do the verification without downloading again?

>
> - manifest.json:
> - "description": "Verify downloaded file", -> please make it clear
> that this verifies a Tails ISO image using a SHA256 checksum. (I think
> sajolida will handle this.)
>
> - origin of files:
> vendor/forge.js: please add a URL of origin for this script as well as
> a version number so that we can update it in the future.
>
> - JSLint http://www.jslint.com/ - this is a tool to write JS code which
> is not error prone and I think it would be nice to follow the
> requirements of JSLint.
>   - Please use double quotes instead of single quotes.
>   - convert tabs to spaces
>   - fart operator =>
>     -  example: this.fetchConf().done()=>{
>        JSLint requires the parens around the parameters, and forbids a {
> left brace after the => fart to avoid syntactic ambiguity. See:
> http://www.jslint.com/help.html#function
>  - replace for loops with foreach (low prio)

>
> - Consider using strict-mode:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/
> Reference/Strict_mode
> We want this code to be forwards compatible as much as possible as
> well as as secure as possible.
>
> - Integrated jquery library. Can we get rid of it and use only vanillaJS?
>
> - Whishlist: please document how to test the extension locally in a
> README file.
> - Exclude this README file from the resulting XPI.
> - See as example the HACKING file in tails
> tails@???:download-and-verify-extension
>
> Cheers!
> u.
>