ma1 pushed to branch tor-browser-115.11.0esr-13.5-1 at The Tor Project / Applications / Tor Browser

Commits: 62619558 by Nuohan Li at 2024-05-09T13:37:32+02:00 Bug 1871109 - generateHash in Manifest.sys.mjs should use sha256 r=peterv, a=dmeehan

Differential Revision: https://phabricator.services.mozilla.com/D204928 - - - - - 4846c1da by Jonathan Kew at 2024-05-09T13:37:33+02:00 Bug 1890204 - Ensure font entry's unitsPerEm and font extents are initialized when gfxFont is created. r=gfx-reviewers,lsalzman

This means that by the time we potentially call GetFontExtents() when drawing, the extents fields are guaranteed to have been been initialized, and there's no risk of the (read-only) access here racing with setting them in UnitsPerEm().

Differential Revision: https://phabricator.services.mozilla.com/D206920 - - - - - 66e2e3ef by Jonathan Kew at 2024-05-09T13:37:34+02:00 Bug 1893891 - Clear mSharedBlobData if blob creation failed. a=dmeehan

Original Revision: https://phabricator.services.mozilla.com/D208983

Differential Revision: https://phabricator.services.mozilla.com/D209209 - - - - -

5 changed files:

- dom/manifest/Manifest.sys.mjs - dom/manifest/test/browser_Manifest_install.js - gfx/thebes/gfxFont.cpp - gfx/thebes/gfxFontEntry.cpp - gfx/thebes/gfxFontEntry.h

Changes:

===================================== dom/manifest/Manifest.sys.mjs ===================================== @@ -29,11 +29,11 @@ ChromeUtils.defineESModuleGetters(lazy, { * @note The generated hash is returned in base64 form. Mind the fact base64 * 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( Ci.nsICryptoHash ); - cryptoHash.init(Ci.nsICryptoHash.MD5); + cryptoHash.init(hashAlg); const stringStream = Cc[ "@mozilla.org/io/string-input-stream;1" ].createInstance(Ci.nsIStringInputStream); @@ -66,11 +66,39 @@ class Manifest { this._manifestUrl = manifestUrl; // 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. - const fileName = generateHash(manifestUrl) + ".json"; - this._path = PathUtils.join(MANIFESTS_DIR, fileName); + const filename = + generateHash(manifestUrl, Ci.nsICryptoHash.SHA256) + ".json"; + this._path = PathUtils.join(MANIFESTS_DIR, filename); 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() { return this._browser; } @@ -80,6 +108,7 @@ class Manifest { }

async initialize() { + await this.removeMD5BasedFilename(); this._store = new lazy.JSONFile({ path: this._path, saveDelayMs: 100 }); await this._store.load(); }

===================================== dom/manifest/test/browser_Manifest_install.js ===================================== @@ -23,18 +23,59 @@ function makeTestURL() { 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 () { 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) { - let manifest = await Manifests.getManifest(browser, manifestUrl); - is(manifest.installed, false, "We haven't installed this manifest yet"); + let tmpManifest = await Manifests.getManifest(browser, manifestUrl); + 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); is(manifest.name, "hello World", "Manifest has correct name"); is(manifest.installed, true, "Manifest is installed"); is(manifest.url, manifestUrl, "has correct url"); 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); is(manifest.installed, true, "New instances are installed");

===================================== gfx/thebes/gfxFont.cpp ===================================== @@ -952,6 +952,10 @@ gfxFont::gfxFont(const RefPtr<UnscaledFont>& aUnscaledFont, }

mKerningSet = HasFeatureSet(HB_TAG('k', 'e', 'r', 'n'), mKerningEnabled); + + // Ensure the gfxFontEntry's unitsPerEm and extents fields are initialized, + // so that GetFontExtents can use them without risk of races. + Unused << mFontEntry->UnitsPerEm(); }

gfxFont::~gfxFont() {

===================================== gfx/thebes/gfxFontEntry.cpp ===================================== @@ -262,14 +262,22 @@ already_AddRefed<gfxFont> gfxFontEntry::FindOrMakeFont( }

uint16_t gfxFontEntry::UnitsPerEm() { + { + AutoReadLock lock(mLock); + if (mUnitsPerEm) { + return mUnitsPerEm; + } + } + + AutoTable headTable(this, TRUETYPE_TAG('h', 'e', 'a', 'd')); + AutoWriteLock lock(mLock); + if (!mUnitsPerEm) { - AutoTable headTable(this, TRUETYPE_TAG('h', 'e', 'a', 'd')); if (headTable) { uint32_t len; const HeadTable* head = reinterpret_cast<const HeadTable*>(hb_blob_get_data(headTable, &len)); if (len >= sizeof(HeadTable)) { - mUnitsPerEm = head->unitsPerEm; if (int16_t(head->xMax) > int16_t(head->xMin) && int16_t(head->yMax) > int16_t(head->yMin)) { mXMin = head->xMin; @@ -277,6 +285,7 @@ uint16_t gfxFontEntry::UnitsPerEm() { mXMax = head->xMax; mYMax = head->yMax; } + mUnitsPerEm = head->unitsPerEm; } }

@@ -286,12 +295,13 @@ uint16_t gfxFontEntry::UnitsPerEm() { mUnitsPerEm = kInvalidUPEM; } } + return mUnitsPerEm; }

bool gfxFontEntry::HasSVGGlyph(uint32_t aGlyphId) { - NS_ASSERTION(mSVGInitialized, - "SVG data has not yet been loaded. TryGetSVGData() first."); + MOZ_ASSERT(mSVGInitialized, + "SVG data has not yet been loaded. TryGetSVGData() first."); return GetSVGGlyphs()->HasSVGGlyph(aGlyphId); }

@@ -309,8 +319,8 @@ bool gfxFontEntry::GetSVGGlyphExtents(DrawTarget* aDrawTarget,

void gfxFontEntry::RenderSVGGlyph(gfxContext* aContext, uint32_t aGlyphId, SVGContextPaint* aContextPaint) { - NS_ASSERTION(mSVGInitialized, - "SVG data has not yet been loaded. TryGetSVGData() first."); + MOZ_ASSERT(mSVGInitialized, + "SVG data has not yet been loaded. TryGetSVGData() first."); GetSVGGlyphs()->RenderGlyph(aContext, aGlyphId, aContextPaint); }

@@ -467,8 +477,9 @@ hb_blob_t* gfxFontEntry::FontTableHashEntry::ShareTableAndGetBlob( HB_MEMORY_MODE_READONLY, mSharedBlobData, DeleteFontTableBlobData); if (mBlob == hb_blob_get_empty()) { // The FontTableBlobData was destroyed during hb_blob_create(). - // The (empty) blob is still be held in the hashtable with a strong + // The (empty) blob will still be held in the hashtable with a strong // reference. + mSharedBlobData = nullptr; return hb_blob_reference(mBlob); }

===================================== gfx/thebes/gfxFontEntry.h ===================================== @@ -538,6 +538,9 @@ class gfxFontEntry {

mozilla::gfx::Rect GetFontExtents(float aFUnitScaleFactor) const { // Flip the y-axis here to match the orientation of Gecko's coordinates. + // We don't need to take a lock here because the min/max fields are inert + // after initialization, and we make sure to initialize them at gfxFont- + // creation time. return mozilla::gfx::Rect(float(mXMin) * aFUnitScaleFactor, float(-mYMax) * aFUnitScaleFactor, float(mXMax - mXMin) * aFUnitScaleFactor,

View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/compare/410bb24...