Verified Commit ac2c9355 authored by Nuohan Li's avatar Nuohan Li Committed by ma1
Browse files

Bug 1871109 - generateHash in Manifest.sys.mjs should use sha256 r=peterv, a=dmeehan 

Differential Revision: https://phabricator.services.mozilla.com/D204928
parent 5cc51272

dom/manifest/Manifest.sys.mjs

+33 −4
Original line number Original line Diff line number Diff line
@@ -29,11 +29,11 @@ ChromeUtils.defineESModuleGetters(lazy, { 
 * @note The generated hash is returned in base64 form.  Mind the fact base64

 * @note The generated hash is returned in base64 form.  Mind the fact base64

 * is case-sensitive if you are going to reuse this code.

 * is case-sensitive if you are going to reuse this code.

 */

 */

function generateHash(aString) {

function generateHash(aString, hashAlg) {

  const cryptoHash = Cc["@mozilla.org/security/hash;1"].createInstance(

  const cryptoHash = Cc["@mozilla.org/security/hash;1"].createInstance(

    Ci.nsICryptoHash

    Ci.nsICryptoHash

  );

  );

  cryptoHash.init(Ci.nsICryptoHash.MD5);

  cryptoHash.init(hashAlg);

  const stringStream = Cc[

  const stringStream = Cc[

    "@mozilla.org/io/string-input-stream;1"

    "@mozilla.org/io/string-input-stream;1"

  ].createInstance(Ci.nsIStringInputStream);

  ].createInstance(Ci.nsIStringInputStream);
@@ -66,11 +66,39 @@ class Manifest { 
    this._manifestUrl = manifestUrl;

    this._manifestUrl = manifestUrl;

    // The key for this is the manifests URL that is required to be unique.

    // The key for this is the manifests URL that is required to be unique.

    // However arbitrary urls are not safe file paths so lets hash it.

    // However arbitrary urls are not safe file paths so lets hash it.

    const fileName = generateHash(manifestUrl) + ".json";

    const filename =

    this._path = PathUtils.join(MANIFESTS_DIR, fileName);

      generateHash(manifestUrl, Ci.nsICryptoHash.SHA256) + ".json";

    this._path = PathUtils.join(MANIFESTS_DIR, filename);

    this.browser = browser;

    this.browser = browser;

  }

  }





  /**

   * See Bug 1871109

   * This function is called at the beginning of initialize() to check if a given

   * manifest has MD5 based filename, if so we remove it and migrate the content to

   * a new file with SHA256 based name.

   * This is done due to security concern, as MD5 is an outdated hashing algorithm and

   * shouldn't be used anymore

   */

  async removeMD5BasedFilename() {

    const filenameMD5 =

      generateHash(this._manifestUrl, Ci.nsICryptoHash.MD5) + ".json";

    const MD5Path = PathUtils.join(MANIFESTS_DIR, filenameMD5);

    try {

      await IOUtils.copy(MD5Path, this._path, { noOverwrite: true });

    } catch (error) {

      // we are ignoring the failures returned from copy as it should not stop us from

      // installing a new manifest

    }



    // Remove the old MD5 based file unconditionally to ensure it's no longer used

    try {

      await IOUtils.remove(MD5Path);

    } catch {

      // ignore the error in case MD5 based file does not exist

    }

  }



  get browser() {

  get browser() {

    return this._browser;

    return this._browser;

  }

  }
@@ -80,6 +108,7 @@ class Manifest { 
  }

  }





  async initialize() {

  async initialize() {

    await this.removeMD5BasedFilename();

    this._store = new lazy.JSONFile({ path: this._path, saveDelayMs: 100 });

    this._store = new lazy.JSONFile({ path: this._path, saveDelayMs: 100 });

    await this._store.load();

    await this._store.load();

  }

  }

dom/manifest/test/browser_Manifest_install.js

+43 −2
Original line number Original line Diff line number Diff line
@@ -23,18 +23,59 @@ function makeTestURL() { 
  return url.href;

  return url.href;

}

}





function generateHash(aString, hashAlg) {

  const cryptoHash = Cc["@mozilla.org/security/hash;1"].createInstance(

    Ci.nsICryptoHash

  );

  cryptoHash.init(hashAlg);

  const stringStream = Cc[

    "@mozilla.org/io/string-input-stream;1"

  ].createInstance(Ci.nsIStringInputStream);

  stringStream.data = aString;

  cryptoHash.updateFromStream(stringStream, -1);

  // base64 allows the '/' char, but we can't use it for filenames.

  return cryptoHash.finish(true).replace(/\//g, "-");

}



const MANIFESTS_DIR = PathUtils.join(PathUtils.profileDir, "manifests");



add_task(async function () {

add_task(async function () {

  const tabOptions = { gBrowser, url: makeTestURL() };

  const tabOptions = { gBrowser, url: makeTestURL() };





  const filenameMD5 = generateHash(manifestUrl, Ci.nsICryptoHash.MD5) + ".json";

  const filenameSHA =

    generateHash(manifestUrl, Ci.nsICryptoHash.SHA256) + ".json";

  const manifestMD5Path = PathUtils.join(MANIFESTS_DIR, filenameMD5);

  const manifestSHAPath = PathUtils.join(MANIFESTS_DIR, filenameSHA);



  await BrowserTestUtils.withNewTab(tabOptions, async function (browser) {

  await BrowserTestUtils.withNewTab(tabOptions, async function (browser) {

    let manifest = await Manifests.getManifest(browser, manifestUrl);

    let tmpManifest = await Manifests.getManifest(browser, manifestUrl);

    is(manifest.installed, false, "We haven't installed this manifest yet");

    is(tmpManifest.installed, false, "We haven't installed this manifest yet");





    await tmpManifest.install();



    // making sure the manifest is actually installed before proceeding

    await tmpManifest._store._save();

    await IOUtils.move(tmpManifest.path, manifestMD5Path);



    let exists = await IOUtils.exists(tmpManifest.path);

    is(

      exists,

      false,

      "Manually moved manifest from SHA256 based path to MD5 based path"

    );

    Manifests.manifestObjs.delete(manifestUrl);



    let manifest = await Manifests.getManifest(browser, manifestUrl);

    await manifest.install(browser);

    await manifest.install(browser);

    is(manifest.name, "hello World", "Manifest has correct name");

    is(manifest.name, "hello World", "Manifest has correct name");

    is(manifest.installed, true, "Manifest is installed");

    is(manifest.installed, true, "Manifest is installed");

    is(manifest.url, manifestUrl, "has correct url");

    is(manifest.url, manifestUrl, "has correct url");

    is(manifest.browser, browser, "has correct browser");

    is(manifest.browser, browser, "has correct browser");

    is(manifest.path, manifestSHAPath, "has correct path");



    exists = await IOUtils.exists(manifestMD5Path);

    is(exists, false, "MD5 based manifest removed");





    manifest = await Manifests.getManifest(browser, manifestUrl);

    manifest = await Manifests.getManifest(browser, manifestUrl);

    is(manifest.installed, true, "New instances are installed");

    is(manifest.installed, true, "New instances are installed");