Open Bug 1336208 Opened 7 years ago Updated 2 months ago

Bundle and whitelist fonts when privacy.resistFingerprinting = true

Categories

(Core :: Graphics: Text, enhancement, P3)

Product:
Core
Component:
Graphics: Text
Type:
enhancement

Tracking

()

People

(Reporter: arthur, Unassigned)

References

Details

(Whiteboard: [fingerprinting][gfx-noted][fp-triaged])

Attachments

(9 files)

Pull request against the kinto-settings deployment configuration.
64 bytes, text/x-github-pull-request
jason
: review+
wezhou
: review+
Details | Review
Bug 1336208 - Part 1: Enable MOZ_BUNDLED_FONTS for MAC platform.
59 bytes, text/x-review-board-request
arthur
: review+
jfkthame
: review+
Details
Bug 1336208 - Part 2: Making the gfxPlatformFontList to use fonts directory in profile folder if it exists for bundled fonts.
59 bytes, text/x-review-board-request
arthur
: review+
Details
Bug 1336208 - Part 3: Adding a new BlocklistClient for font fingerprinting.
59 bytes, text/x-review-board-request
arthur
: review+
leplatrem
: review+
Details
Bug 1336208 - Part 4: Add a initial fonts list file which ships with the release.
59 bytes, text/x-review-board-request
Details
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.
59 bytes, text/x-review-board-request
arthur
: review+
jfkthame
: review+
Gijs
: review+
Details
Bug 1336208 - Part 6: Apply the font whitelist when pref 'privacy.resistFingerprinting' is true.
59 bytes, text/x-review-board-request
arthur
: review+
Details
Bug 1336208 - Part 7: Add a test case for testing fonts downloading for fingerprinting resistance.
59 bytes, text/x-review-board-request
arthur
: review+
leplatrem
: review+
Details
PR #1130 ­— Add fingerprinting-defenses buckets to monitor changes configuration.
65 bytes, text/x-github-pull-request
wezhou
: review+
jason
: review+
Details | Review 
In bug 1121643 we uplifted a font whitelisting mechanism from Tor Browser. As a next step, we could include a standard font whitelist that can be automatically activated when the user enables the privacy.resistFingerprinting pref.

Tor Browser's current font whitelists are as follows:
Mac:
pref("font.system.whitelist", "AppleGothic, Apple Color Emoji, Arial, Courier, Geneva, Georgia, Heiti TC, Helvetica, Helvetica Neue, .Helvetica Neue DeskInterface, Hiragino Kaku Gothic ProN, Lucida Grande, Monaco, Noto Sans Armenian, Noto Sans Bengali, Noto Sans Canadian Aboriginal, Noto Sans Cherokee, Noto Sans Devanagari, Noto Sans Ethiopic, Noto Sans Gujar\
ati, Noto Sans Gurmukhi, Noto Sans Kannada, Noto Sans Khmer, Noto Sans Lao, Noto Sans Malayalam, Noto Sans Mongolian, Noto Sans Myanmar, Noto Sans Oriya, Noto Sans Sinhala, Noto Sans Tamil, Noto Sans Telugu, Noto Sans Thaana, Noto Sans Tibetan, Noto Sans Yi, STHeiti, STIX Math, Tahoma, Thonburi, Times, Times New Roman, Verdana");

Windows:
pref("font.system.whitelist", "Arial, Batang, 바탕, Cambria Math, Courier New, Euphemia, Gautami, Georgia, Gulim, 굴림, GulimChe, 굴림체, Iskoola Pota, Kalinga, Kartika, Latha, Lucida Console, MS Gothic, ＭＳ ゴシック, MS Mincho, ＭＳ 明朝, MS PGothic, ＭＳ Ｐゴシック, MS PMincho, ＭＳ Ｐ明朝, MV Boli, Malgun Gothic, Mangal, Meiryo, Meiryo UI, Microsoft Himal\
aya, Microsoft JhengHei, Microsoft JengHei UI, Microsoft YaHei, 微软雅黑, Microsoft YaHei UI, MingLiU, 細明體, Noto Sans Buginese, Noto Sans Khmer, Noto Sans Lao, Noto Sans Myanmar, Noto Sans Yi, Nyala, PMingLiU, 新細明體, Plantagenet Cherokee, Raavi, Segoe UI, Shruti, SimSun, 宋体, Sylfaen, Tahoma, Times New Roman, Tunga, Verdana, Vrinda, Yu Gothic UI");

In addition, we bundle some or all fonts on Windows, Mac, and Linux using the build flag "--enable-bundled-fonts".

Windows bundled fonts (the rest of fonts on the whitelist are system fonts):
NotoSansKhmer-Regular.ttf NotoSansLao-Regular.ttf NotoSansMyanmar-Regular.ttf
NotoSansBuginese-Regular.ttf NotoSansYi-Regular.ttf

Mac bundle (the rest of fonts on the whitelist are system fonts):
NotoSansArmenian-Regular.ttf NotoSansBengali-Regular.ttf NotoSansDevanagari-Regular.ttf NotoSansEthiopic-Regular.ttf NotoSansGujarati-Regular.ttf NotoSansGurmukhi-Regular.ttf NotoSansKannada-Regular.ttf NotoSansKhmer-Regular.ttf NotoSansLao-Regular.ttf NotoSansMalayalam-Regular.ttf NotoSansMyanmar-Regular.ttf NotoSansOriya-Regular.ttf NotoSansSinhala-Regular.ttf NotoSansTamil-Regular.ttf NotoSansTelugu-Regular.ttf NotoSansThaana-Regular.ttf NotoSansTibetan-Regular.ttf"
NotoSansCanadianAboriginal-Regular.ttf NotoSansBuginese-Regular.ttf NotoSansCherokee-Regular.ttf NotoSansMongolian-Regular.ttf NotoSansYi-Regular.ttf
STIXMath-Regular.otf
  
Linux bundle (we use fontconfig to force only bundled fonts to be loaded):
Arimo-Regular.ttf Arimo-Bold.ttf Arimo-Italic.ttf Arimo-BoldItalic.ttf Cousine-Regular.ttf Tinos-Regular.ttf Tinos-Bold.ttf Tinos-Italic.ttf Tinos-BoldItalic.ttf NotoNaskhArabic-Regular.ttf NotoSansArmenian-Regular.ttf NotoSansBengali-Regular.ttf NotoSansDevanagari-Regular.ttf NotoSansEthiopic-Regular.ttf NotoSansGeorgian-Regular.ttf NotoSansGujarati-Regular.ttf NotoSansGurmukhi-Regular.ttf NotoSansHebrew-Regular.ttf NotoSansKannada-Regular.ttf NotoSansKhmer-Regular.ttf NotoSansLao-Regular.ttf NotoSansMalayalam-Regular.ttf NotoSansMyanmar-Regular.ttf NotoSansOriya-Regular.ttf NotoSansSinhala-Regular.ttf NotoSansTamil-Regular.ttf NotoSansTelugu-Regular.ttf NotoSansThaana-Regular.ttf NotoSansThai-Regular.ttf NotoSansTibetan-Regular.ttf NotoSerifArmenian-Regular.ttf NotoSerifKhmer-Regular.ttf NotoSerifLao-Regular.ttf NotoSerifThai-Regular.ttf
NotoSansCanadianAboriginal-Regular.ttf NotoSansBuginese-Regular.ttf NotoSansCherokee-Regular.ttf NotoSansMongolian-Regular.ttf NotoSansYi-Regular.ttf
STIXMath-Regular.otf 
NotoEmoji-Regular.ttf
NotoSansJP-Regular.otf NotoSansKR-Regular.otf NotoSansSC-Regular.otf NotoSansTC-Regular.otf
Whiteboard: [tor][fingerprinting] → [tor][fingerprinting][gfx-noted]
(In reply to Arthur Edelstein [:arthuredelstein] from comment #0)
> As a next step, we could include a standard font whitelist that can be
> automatically activated when the user enables the
> privacy.resistFingerprinting pref.

If (like Tor) we want the whitelist to include a bunch of bundled Noto fonts, not _just_ standard system fonts that we can expect to be available, that will have a substantial impact on the download size (and installed footprint) of Firefox for all users.

Maybe it's worth that size increase -- I'm not sure. But it's something to take into consideration. How much size increase are we prepared to accept for a given decrease in fingerprintability?
(In reply to Jonathan Kew (:jfkthame) from comment #1)
> (In reply to Arthur Edelstein [:arthuredelstein] from comment #0)
> > As a next step, we could include a standard font whitelist that can be
> > automatically activated when the user enables the
> > privacy.resistFingerprinting pref.
> 
> If (like Tor) we want the whitelist to include a bunch of bundled Noto
> fonts, not _just_ standard system fonts that we can expect to be available,
> that will have a substantial impact on the download size (and installed
> footprint) of Firefox for all users.

That's absolutely true. Here are the installed footprints for bundled fonts in Tor Browser (uncompressed):
Linux: 32 MB
Mac: 3.4 MB
Windows: 372 kB.

For all three, the compression ratio in the download file is roughly 50%. I checked on Linux and the extra weight due to 17 MB (with a total Tor Browser download size of 68 MB).

Of the 32 MB of fonts on Linux, 23 MB belong to four CJK fonts (Simplified Chinese, Traditional Chinese, Japanese, Korean). I imagine it might be possible to improve this substantially by removing seldom-used or obsolete code points. For example, I found a character frequency list for Chinese here: http://lingua.mtsu.edu/chinese-computing/statistics/char/list.php?Which=MO

Each of the Noto CJK fonts reportedly contains 65,536 glyphs, so even if we retained the top 10,000 code points for Chinese given in that frequency list, we would have a substantial savings. (Having a slightly less comprehensive character coverage for CJK languages might be acceptable for an anti-fingerprinting mode.)
Priority: -- → P2
Priority: P2 → P1
Whiteboard: [tor][fingerprinting][gfx-noted] → [tor][fingerprinting][gfx-noted][fp:m1]
Whiteboard: [tor][fingerprinting][gfx-noted][fp:m1] → [tor][fingerprinting][gfx-noted][fp:m2]
In my opinion, I think Firefox can download these fonts instead of bundling them since we are not likely to increase the size of the installation file. But, I am not sure about could we do this and how to do this?

Could you provide some insight regarding this, jfkthame?
Flags: needinfo?(jfkthame)
We already do this (for the serif and sans-serif faces that we want to "bundle" as preferred fonts) on Android; see bug 1194338 and related bugs. I assume we could use a similar mechanism on desktop, with the same server backend providing the resources, though we'd need a separate implementation of the client code as the Fennec version is all Java, I believe.

The Firefox application would need to be built with the MOZ_BUNDLED_FONTS option, which we currently set on Windows and Linux (IIRC) but not on OS X; then it's just a matter of downloading the files and placing them in the application's /fonts/ directory, and calling nsIFontEnumerator.updateFontList() when a new font is added, so that gecko will update its list.

So I think what you'd need to do is:
(a) Figure out any UI issues (e.g. if checking a "[x] resist fingerprinting" checkbox is going to trigger downloading several MB of fonts, should the user be made aware of this? or do we just do it silently in the background with no UI?)
(b) Figure out setup/management/maintenance of the server providing the font resources -- talk to the Fennec people already doing this for their fonts.
(c) Get MOZ_BUNDLED_FONTS enabled for all builds/channels/platforms where this is wanted.
(d) Implement a JS version of the downloadable content (DLC) client code, equivalent to Fennec's Java version -- actually, this may already exist for other content such as add-on blocklists? If so, maybe we just need to add support for a new type of content, so it knows what to do with the font resources when it downloads them.
(e) Test, test, and test some more! :)
Flags: needinfo?(jfkthame)
Assignee: nobody → tihuang
Hi, Tarek

We think Kinto could be helpful for this font downloading. Would you provide support for us on this?

I think we need your help on ...

* Create a account for us to create bucket and collection on Firefox setting server.
* A server to host fonts data, like what Fennec did on their fonts DLC.

Thanks.
Flags: needinfo?(tarek)
Hi Tim, forwarding the request to Benson
Flags: needinfo?(tarek) → needinfo?(bwong)
Forwarding to :natim for assistance with kinto, fonts and desktop integration.
Flags: needinfo?(bwong) → needinfo?(rhubscher)
Hi Tim,

I would be glad to help you setup all of this.
Can we setup a meeting to talk about that?

As a first step could you also provide me with the ldap email address of people who would need to update and review the font list updates?

Thanks,

Rémy
Flags: needinfo?(rhubscher) → needinfo?(tihuang)
Thanks, Remy. I've sent a mail to you.
Flags: needinfo?(tihuang)
I think I will use the current blocklist implementation to handle the communication between Firefox and Kinto server. Firefox will use blocklist to get the fonts list from the Kinto server and use nsRFPService to download fonts from the server. So, I will add a new BlocklistClient and a new collection under the bucket 'blocklists'.

Mark, what do you think about this? Could we use blocklist for this?
Flags: needinfo?(mgoodwin)
Hi Tim,
This is indeed the easiest way to get started!

The term «blocklist» refers to something in particular in Firefox, but in the code — as you may have noticed — anything beyond the `processCallback` is generic. 

That's why in the long-go, your use-case justifies to generalize the naming and the current code. But we can do that as a last step, and it shouldn't prevent you to start.

In your case, you would have:

* something like a `catalog` bucket, and a `fonts` collection (for example)
* a new instance of `BlocklistClient` with a specific callback function that uses nsRFPService and no signature verification

Once you have the basics working with an instance of Kinto running on your machine, you can:

* create those bucket/collection in our dev instance 
* setup review workflow for those collections (remy or I can help)
* enable signature verification in the client

And then, we can (in separate bugs or not):

* rename the blocklist notions to something more generic (e.g. remote settings)
* change the way we trigger the remote settings synchronization to something more generic — currently initiated from nsBlocklistService::notify()) — (rhelmer could help here)
Thanks, Mathieu. Your advice is really helpful. 

After talked with Remy. We are going to create a 'fingerprinting-defenses' bucket and a 'fonts' collection for this. And we will use the blocklist-clients.js to sync up the fonts list for Firefox.
Flags: needinfo?(mgoodwin)
I started a pull-request with the mandatory changes to the kinto-settings stack.
Can we deploy them to stage?

Chris would you be able duplicate the tests you have for the fennec/catalog for this new use case?


As a summary of what happened by email:

- Bucket name: fingerprinting-defenses
- Collection name: fonts
- editors and reviewers group members: 
  - Tim Huang, tihuang@mozilla.com
  - Chung-Sheng Fu, cfu@mozilla.com
  - Ethan Tseng, ettseng@mozilla.com
  - Tom Ritter, tom@mozilla.com
- JSONSchema:

{
 "properties": {
    "platforms": {
        "type": "array",
        "title": "Platforms",
        "description": "The list of supported platforms for this font.",
        "items": {
          "type": "string",
          "title": "Platforms",
          "enum": [
               "win",
               "macosx",
               "linux"
         ]
      }
    }
 },
 "type": "object"
}
Flags: needinfo?(chartjes)
Attachment #8893404 - Flags: review?(wezhou)
Attachment #8893404 - Flags: review?(jthomas)
Julien, could you create a new certificate for this usecase and upgrade autograph config so that we can enable content-signature for this new collection?

We can plan to make the switch to autograph2 in the meantime if this helps with the autograph config.
Flags: needinfo?(jvehent)
Will do. Please make the move to autograph2 first, that's where the new configuration will live.
Attachment #8893404 - Flags: review?(jthomas) → review+
Attachment #8893404 - Flags: review?(wezhou) → review+
Whiteboard: [tor][fingerprinting][gfx-noted][fp:m2] → [tor][fingerprinting][gfx-noted][fp:m3]
It has been deployed to stage but apparently the autograph 2 heartbeat is failing.

https://kinto-writer.stage.mozaws.net/v1/__heartbeat__
Autograph configuration has been fixed, this should be good to go from the signing side, just holler when ready to deploy prod.
Flags: needinfo?(jvehent)
Hi Mathieu,

I've noticed that nsBlocklistService::notify() is trigger by nsIUpdateTimerManager and it happens every 86400 seconds. But, it seems that this timer won't be triggered when the browser has been started up at the first time. It has to wait until the timer expires. If this is true, does that mean that the blocklist will be out-of-date for the first 86400 seconds after browser starts?

And is there any way that we can trigger this timer at browser starts?

Thanks.
Flags: needinfo?(mathieu)
Hi Tim,

I am not familiar enough with that, but I suppose it takes the time of the last notification into account, so that if you always open your browser for less than a day you still get updates.

> And is there any way that we can trigger this timer at browser starts?

Probably yes, but I see that the trend is more to get things out of startup than the opposite (new users first impression etc.).

If your concern is about having empty data on new profiles, you could ship an initial dataset with the release maybe?

The hook on blocklist notification is kind of a hack anyway. We should probably consider registering a dedicated one for remote settings. 

Let us know :)
Flags: needinfo?(mathieu)
Comment on attachment 8901696 [details]
Bug 1336208 - Part 4: Add a initial fonts list file which ships with the release.

https://reviewboard.mozilla.org/r/173124/#review178436

::: toolkit/components/resistfingerprinting/fonts.json:1
(Diff revision 1)
> +{"data":[]}

This file still remains empty since the kinto server is under deploying. We will update this after the kinto server is ready.
Comment on attachment 8901698 [details]
Bug 1336208 - Part 6: Apply the font whitelist when pref 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/173128/#review178440

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:71
(Diff revision 1)
>  #define RFP_FONTS_LIST_FILE "fonts.json"
>  #define RFP_DOWNLOAD_FONTS_MSG  "resist-fingerprinting:download-fonts"
>  #define RFP_DOWNLOAD_FONTS_SUCCESS  "resist-fingerprinting:download-fonts-success"
>  #define RFP_DOWNLOAD_FONTS_FAILURE  "resist-fingerprinting:download-fonts-failure"
>  
> +// The font whitelist.

The font white list should be updated given that the recent change of default JP fonts. We will update this after Tor has their new fonts list.
Comment on attachment 8901695 [details]
Bug 1336208 - Part 3: Adding a new BlocklistClient for font fingerprinting.

https://reviewboard.mozilla.org/r/173122/#review178442

::: services/common/blocklist-clients.js:498
(Diff revision 1)
> +this.FontsRFPClient = new BlocklistClient(
> +  Services.prefs.getCharPref(PREF_RFP_FONT_COLLECTION),
> +  PREF_RFP_FONT_CHECKED_SECONDS,
> +  (records) => updateFontList(this.FontsRFPClient.filename, records),
> +  Services.prefs.getCharPref(PREF_RFP_FONT_BUCKET)
> +);

We should use a content signature here, Although, the kinto server is under deploying as well as the signature. So, we leave this without a content signature for now, and we will add the signature after it's been deployed.
Comment on attachment 8901693 [details]
Bug 1336208 - Part 1: Enable MOZ_BUNDLED_FONTS for MAC platform.

https://reviewboard.mozilla.org/r/173118/#review178696
Attachment #8901693 - Flags: review?(arthuredelstein) → review+
Comment on attachment 8901694 [details]
Bug 1336208 - Part 2: Making the gfxPlatformFontList to use fonts directory in profile folder if it exists for bundled fonts.

https://reviewboard.mozilla.org/r/173120/#review178698
Attachment #8901694 - Flags: review?(arthuredelstein) → review+
(In reply to Rémy Hubscher (:natim) from comment #14)

> 
> Chris would you be able duplicate the tests you have for the fennec/catalog
> for this new use case?

Sorry for the delay in answering (I was away on PTO since Aug 3).

Yes, I should be able to duplicate my tests to include checking for this new use case.
Flags: needinfo?(chartjes)
Comment on attachment 8901695 [details]
Bug 1336208 - Part 3: Adding a new BlocklistClient for font fingerprinting.

https://reviewboard.mozilla.org/r/173122/#review178964

::: services/common/blocklist-clients.js:450
(Diff revision 1)
> +
> +  const serialized = JSON.stringify({data: records}, null, 2);
> +
> +  try {
> +    await OS.File.writeAtomic(path, serialized, {tmpPath: path + ".tmp"});
> +    Services.obs.notifyObservers(null, "resist-fingerprinting:download-fonts");

Should we use a file on disk, or could you pass the new records along with the notification?
Attachment #8901695 - Flags: review?(mathieu) → review+
The configuration referenced in comment 14 has been deployed to stage. Users should also now have access to the kinto stage endpoint via VPN.
Apparently we don't have the permission to write in the collection due to a wrong group name. (my fault)

I have created https://github.com/mozilla-services/cloudops-deployment/pull/1078 to fix it.
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review179544

This is excellent work but it's a lot of complex code. It's difficult for a reader to follow the control flow. I think, given that this is not performance-critical code, a JavaScript module that performs the same function could be much simpler and more concise, and, especially if it were to use async/await, considerably easier to read and debug.

::: commit-message-5fb9d:5
(Diff revision 1)
> +Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance. r?jfkthame,arthuredelstein
> +
> +This patch enables the nsRFPService to download fonts according to the fonts list
> +which is downloaded through Kinto Server. The nsRFPService will try to download
> +fonts when 'privacy.resistFingerprinting' flipping to true and update the font

nit: is flipped

::: commit-message-5fb9d:6
(Diff revision 1)
> +Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance. r?jfkthame,arthuredelstein
> +
> +This patch enables the nsRFPService to download fonts according to the fonts list
> +which is downloaded through Kinto Server. The nsRFPService will try to download
> +fonts when 'privacy.resistFingerprinting' flipping to true and update the font
> +list and the font whitelist. The service will first check that whether local fonts

nit: check whether

::: commit-message-5fb9d:7
(Diff revision 1)
> +
> +This patch enables the nsRFPService to download fonts according to the fonts list
> +which is downloaded through Kinto Server. The nsRFPService will try to download
> +fonts when 'privacy.resistFingerprinting' flipping to true and update the font
> +list and the font whitelist. The service will first check that whether local fonts
> +exist and are they up-to-date. If there are up-to-date, we will skip the download.

nit: and if they are up to date. If they are up to date,

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:85
(Diff revision 1)
>  static uint32_t sVideoFramesPerSec;
>  static uint32_t sVideoDroppedRatio;
>  static uint32_t sTargetVideoRes;
>  
> +nsCString
> +ToHexString(const nsCString& aStr) {

Instead of duplicating this function from elsewhere in the codebase, can we refactor so that it is only defined once?
Attachment #8901697 - Flags: review?(arthuredelstein)
Comment on attachment 8901699 [details]
Bug 1336208 - Part 7: Add a test case for testing fonts downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173130/#review179574

Tim -- looks great.
Attachment #8901699 - Flags: review?(arthuredelstein) → review+
Comment on attachment 8901698 [details]
Bug 1336208 - Part 6: Apply the font whitelist when pref 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/173128/#review179568

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:73
(Diff revision 1)
>  #define RFP_DOWNLOAD_FONTS_SUCCESS  "resist-fingerprinting:download-fonts-success"
>  #define RFP_DOWNLOAD_FONTS_FAILURE  "resist-fingerprinting:download-fonts-failure"
>  
> +// The font whitelist.
> +#ifdef XP_WIN
> +#define RFP_FONTS_WHITELIST "Arial, Batang, 바탕, Cambria Math, Courier New, \

Is it safe to paste unicode font names into a vanilla C++ string literal?
Attachment #8901698 - Flags: review?(arthuredelstein) → review+
Comment on attachment 8901695 [details]
Bug 1336208 - Part 3: Adding a new BlocklistClient for font fingerprinting.

https://reviewboard.mozilla.org/r/173122/#review178700

::: services/common/blocklist-clients.js:54
(Diff revision 1)
>  const PREF_BLOCKLIST_ENFORCE_SIGNING         = "services.blocklist.signing.enforced";
>  
> +// Prefs for font fingerprinting resistance.
> +const PREF_RFP_FONT_BUCKET          = "privacy.resistFingerprinting.fonts.bucket";
> +const PREF_RFP_FONT_CHECKED_SECONDS = "privacy.resistFingerprinting.fonts.checked";
> +const PREF_RFP_FONT_COLLECTION      = "privacy.resistFingerprinting.fonts.collection";

Nit: Maybe make these `PREF_RFP_FONTS_*` (plural fonts) to match the pref name string?
Attachment #8901695 - Flags: review?(arthuredelstein) → review+
Attachment #8901697 - Flags: review?(jfkthame)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review179544

That's a good point. I agree that writing this into JS module would be more simpler and readable for reviewers. So, I will move the implementation to a JS module. Clearing review flags for now.
Comment on attachment 8901699 [details]
Bug 1336208 - Part 7: Add a test case for testing fonts downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173130/#review179902

::: browser/components/resistfingerprinting/test/browser/browser_fonts_downloading.js:79
(Diff revision 1)
> +    adapter: FirefoxAdapter,
> +    adapterOptions: {aSqliteHandle},
> +    bucket: "fingerprinting-defenses"
> +  };
> +  return new Kinto(config).collection(aCollectionName);
> +}

Does not seem to be used. (Note: see Bug 1377533)
Attachment #8901699 - Flags: review?(mathieu) → review+
Comment on attachment 8901694 [details]
Bug 1336208 - Part 2: Making the gfxPlatformFontList to use fonts directory in profile folder if it exists for bundled fonts.

https://reviewboard.mozilla.org/r/173120/#review179984

I'm not entirely comfortable with this.... AFAICS, it means that the "bundled" fonts (whether truly "bundled" with the browser, or added via the DLC service) will end up getting re-activated each time InitFontListForPlatform() is called (e.g. if the whitelist pref is modified, or if the user installs or removes a font locally); but those font resources never get deactivated.

I don't know if there's any harm in repeatedly "registering" the same font URLs with the Core Text font manager using CTFontManagerRegisterFontsForURL, yet never unregistering them; I'm guessing it's probably benign, but it would be nice to have some confirmation of that.

More importantly, though, once we have a potentially mutable collection of "bundled" fonts (e.g. resources in the downloaded collection might get replaced by new versions) we do need to consider how to unregister fonts that are no longer available. So I think ActivateBundledFonts needs to keep a record of the files it has registered, allowing us to unregister them when they're not there any longer.

(Relatedly, how will updates to the downloadable fonts be handled? Will the file names be versioned, so that an update will always appear as a new, distinct resource URL, or will they be updated in-place? If the latter, it may be necessary to unregister and re-register the resource even if its URL hasn't changed, otherwise Core Text may have cached no-longer-valid data.)
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #0)

> Mac bundle (the rest of fonts on the whitelist are system fonts):
> NotoSansArmenian-Regular.ttf NotoSansBengali-Regular.ttf
> NotoSansDevanagari-Regular.ttf NotoSansEthiopic-Regular.ttf
> NotoSansGujarati-Regular.ttf NotoSansGurmukhi-Regular.ttf
> NotoSansKannada-Regular.ttf NotoSansKhmer-Regular.ttf
> NotoSansLao-Regular.ttf NotoSansMalayalam-Regular.ttf
> NotoSansMyanmar-Regular.ttf NotoSansOriya-Regular.ttf
> NotoSansSinhala-Regular.ttf NotoSansTamil-Regular.ttf
> NotoSansTelugu-Regular.ttf NotoSansThaana-Regular.ttf
> NotoSansTibetan-Regular.ttf"
> NotoSansCanadianAboriginal-Regular.ttf NotoSansBuginese-Regular.ttf
> NotoSansCherokee-Regular.ttf NotoSansMongolian-Regular.ttf
> NotoSansYi-Regular.ttf
> STIXMath-Regular.otf

BTW, note that Apple ships a whole bunch of Noto fonts for various languages, in "/Library/Application Support/Apple/Fonts/Language Support/", and as of bug 1392659, Firefox now activates and uses these automatically. So this should greatly reduce the need to bundle or download additional fonts on macOS.
(In reply to Jonathan Kew (:jfkthame) from comment #44)

> BTW, note that Apple ships a whole bunch of Noto fonts for various
> languages, in "/Library/Application Support/Apple/Fonts/Language Support/",
> and as of bug 1392659, Firefox now activates and uses these automatically.
> So this should greatly reduce the need to bundle or download additional
> fonts on macOS.

On what versions of macOS are these Noto fonts available? I think we still need to bundle these additional fonts if they're not available in all (earlier) macOS versions that we are still supporting.
(In reply to Jonathan Kew (:jfkthame) from comment #43)
> Comment on attachment 8901694 [details]
> Bug 1336208 - Part 2: Making the nsIFontEnumerator.updateFontList() also
> updates bundled fonts for MAC platform.
> 
> https://reviewboard.mozilla.org/r/173120/#review179984
> 
> (Relatedly, how will updates to the downloadable fonts be handled? Will the
> file names be versioned, so that an update will always appear as a new,
> distinct resource URL, or will they be updated in-place? If the latter, it
> may be necessary to unregister and re-register the resource even if its URL
> hasn't changed, otherwise Core Text may have cached no-longer-valid data.)

The font files will be updated in-place. So, I will implement the mechanism for unregistering and reregistering font resources for MAC. Although, Do we need a similar mechanism for Windows and Linux? I think we need this for Linux that we have to call FcConfigAppFontClear() before [1]. And for GDI implementation of Windows, we need to call RemoveFontResourceExW() before [2].
But, I am not sure about the DirectWrite implementation of Windows that it looks like there is no unregister function here, or maybe I missed it. 

Do you have any idea about this, Jonathan?


[1] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxFcPlatformFontList.cpp#2002
[2] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxGDIFontList.cpp#1211
Flags: needinfo?(jfkthame)
In DirectWrite, there isn't a mechanism of "registering" additional fonts as part of a font list managed by the system font service, which is more-or-less what's happening on the other platforms. With DW, on the other hand, the "bundled" fonts are enumerated in a separate list that is private to Gecko, and manages the IDWriteFontFile objects that reference the underlying files. See the two IDWriteFontCollection objects (mSystemFonts and mBundledFonts) owned by gfxDWriteFontList.

So AFAICS, all that needs to happen here is that the collection referenced by mBundledFonts is destroyed and a new one created, which will provide a fresh and up-to-date collection of IDWriteFontFiles.
Flags: needinfo?(jfkthame)
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #45)
> (In reply to Jonathan Kew (:jfkthame) from comment #44)
> 
> > BTW, note that Apple ships a whole bunch of Noto fonts for various
> > languages, in "/Library/Application Support/Apple/Fonts/Language Support/",
> > and as of bug 1392659, Firefox now activates and uses these automatically.
> > So this should greatly reduce the need to bundle or download additional
> > fonts on macOS.
> 
> On what versions of macOS are these Noto fonts available? I think we still
> need to bundle these additional fonts if they're not available in all
> (earlier) macOS versions that we are still supporting.

I don't know the history of them... I only became aware of their presence fairly recently (on macOS 10.12), but I don't have ready access to older versions to see whether they were around earlier.
Hi, Jonathan.

I am wondering that does the chrome process and the content process has it own instance of the font list respectively? So, when updating the font list and applying the whitelist, should we have to call nsIFontEnumerator.UpdateFontList() on both the chrome process and the content process?
Flags: needinfo?(jfkthame)
Yes, each process has its own instance of the list; content processes normally get the list by querying . (Font selection during reflow is much too hot of a codepath to allow for some kind of IPC where content would have to ask the chrome process about fonts.) So all processes need to update their list when there's a change.

(Depending on the platform, the content process may enumerate available fonts for itself, or it may send an IPC query to the chrome process to retrieve the list from there. But that's just an implementation detail; in either case, all processes need to re-do InitFontList to get a fresh list.)
Flags: needinfo?(jfkthame)
Hi, Jonathan.

After inspecting the code, I believe that the content process doesn't get font list from the chrome process in MAC OS. See [1], the SystemFontFamilyList does get updated in this function and I think it is supposed to be. So, the content process will still ask Core Text for fonts. Please correct me if I am wrong.

And I have a question regarding making UpdateFontList() takes effect on the fly for MAC OS. Since the font list for the content process will only get fonts from the chrome process via an IPC message during the initaliztion. So, we need to add another IPC message to allow the content process to ask the chrome process for font list. Or maybe we don't have to do anything since the SystemFontFamilyList will be cleared[2] after initaliztion. So, the content process will query Core Text directly for fonts when calling nsIFontEnumerator.UpdateFontList() after initaliztion. And this could be fine in terms of performance due to nsIFontEnumerator.UpdateFontList() won't be called so often.

I don't have a clear answer here, could you provide your insight regarding this?

[1] http://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1129
[2] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxMacPlatformFontList.mm#881
Flags: needinfo?(jfkthame)
(In reply to Tim Huang[:timhuang] from comment #51)
> Hi, Jonathan.
> 
> After inspecting the code, I believe that the content process doesn't get
> font list from the chrome process in MAC OS. See [1], the
> SystemFontFamilyList does get updated in this function and I think it is
> supposed to be. So, the content process will still ask Core Text for fonts.
> Please correct me if I am wrong.

I'm a bit confused by this at the moment; it's possible we have broken this at some point since it was originally implemented. I'd like to look into this a bit more. Leaving needinfo? for now to remind me to dig in...
(In reply to Jonathan Kew (:jfkthame) from comment #52)
> (In reply to Tim Huang[:timhuang] from comment #51)
> > Hi, Jonathan.
> > 
> > After inspecting the code, I believe that the content process doesn't get
> > font list from the chrome process in MAC OS. See [1], the
> > SystemFontFamilyList does get updated in this function and I think it is
> > supposed to be. So, the content process will still ask Core Text for fonts.
> > Please correct me if I am wrong.
> 
> I'm a bit confused by this at the moment; it's possible we have broken this
> at some point since it was originally implemented. I'd like to look into
> this a bit more. Leaving needinfo? for now to remind me to dig in...

It's definitely broken. :( By instrumenting a build, I confirmed that immediately after bug 1314932 landed, we -did- use the font list passed from the chrome process to initialize the child; and in current trunk builds, we don't.

Looking at telemetry for InitFontList[1] we can clearly see an improvement early in the Nightly 53 cycle, which corresponds to when bug 1314932 landed; and then part-way through Nightly 54, that improvement is lost, as a result of the changes in bug 1303096.

I've filed bug 1399503 for this regression.

[1] https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median!5th-percentile!95th-percentile&cumulative=0&e10s=true&end_date=null&keys=&max_channel_version=nightly%252F57&measure=MAC_INITFONTLIST_TOTAL&min_channel_version=nightly%252F51&os=Darwin&processType=content&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0
Flags: needinfo?(jfkthame)
Comment on attachment 8907447 [details] [review]
PR #1130 ­— Add fingerprinting-defenses buckets to monitor changes configuration.

LGTM.
Attachment #8907447 - Flags: review?(jthomas) → review+
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review184502

Looks good to me. My only suggestion is to try to shorten the code in FontsDownloader.js by using some existing library functions.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:32
(Diff revision 2)
> + * A helper function for loading local file.
> + *
> + * @param {nsIFile} aFile The nsIFile of the local file.
> + * @return {String} The data of the local file.
> + */
> +async function fetchFromFile(aFile) {

Can you use OS.File.read(...) instead of this custom function? Also, I think if you only want to compute a hash, you can use the Uint8Array file data as input to the hash function; no need to convert to a string first.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:66
(Diff revision 2)
> + * @param {nsIURI} aURI        The URL to be fetched.
> + * @param {String} aOriginType The original data type of the data.
> + * @param {String} aTargetType The target data type of the data.
> + * @return {String} The converted data.
> + */
> +async function fetchAndConvert(aURI, aOriginType, aTargetType) {

Instead of adding this function, can you use XMLHttpRequest or the fetch API? I think they likely perform gzip decompression transparently. And if you use the arraybuffer result, you don't need to convert the file from binary to string and back to binary again. You can hash and save the binary data directly.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:124
(Diff revision 2)
> + *
> + * @param {String} aData The data to be verified.
> + * @param {String} aHash The hash value
> + * @return {boolean} A boolean indicates whether hash matches.
> + */
> +function verifyHash(aData, aHash) {

Maybe you can use
```javascript
Cu.importGlobalProperties(['crypto']);
let hashData = new Uint8Array(await crypto.subtle.digest("SHA-256", aData));
```

::: toolkit/components/resistfingerprinting/FontsDownloader.js:167
(Diff revision 2)
> +    } else if (aTopic === "xpcom-shutdown") {
> +      this.uninit();
> +    }
> +  },
> +
> +  maybeDownloadFonts() {

Perhaps this would be cleaner as an async function.
Attachment #8901697 - Flags: review?(arthuredelstein) → review+
Can this wait for 58 before landing?
Comment on attachment 8901695 [details]
Bug 1336208 - Part 3: Adding a new BlocklistClient for font fingerprinting.

https://reviewboard.mozilla.org/r/173122/#review178964

> Should we use a file on disk, or could you pass the new records along with the notification?

We need to use a file here since the downloading will only happen when 'privacy.resistFingerprinting' is true. But, this pref could be false when the fonts list get updated. So, we need to put the list into a file. We can directly accesss this file for the up-to-date font list when the pref is flipping to true.
(In reply to Milan Sreckovic [:milan] from comment #64)
> Can this wait for 58 before landing?

Sure, that's what we are planning to do. Thanks.
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review184502

> Instead of adding this function, can you use XMLHttpRequest or the fetch API? I think they likely perform gzip decompression transparently. And if you use the arraybuffer result, you don't need to convert the file from binary to string and back to binary again. You can hash and save the binary data directly.

Unfortunately, We have to use this function since XHR and fetch API only decompress data if the HTTP header 'Content-Encoding' is presented, but this header doesn't exist for fonts download location on Kinto server. Therefore, we need this function to download and decompress fonts.
Comment on attachment 8901694 [details]
Bug 1336208 - Part 2: Making the gfxPlatformFontList to use fonts directory in profile folder if it exists for bundled fonts.

https://reviewboard.mozilla.org/r/173120/#review193096

The use of the sequence

    DeactivateBundledFonts();
    ActivateBundledFonts();

in the InitFontListForPlatform() methods (for both gfxGDIFontList and gfxMacPlatformFontList) means that the ActivateBundledFonts call they do in their constructors becomes pointless, doesn't it? So please remove that, to reduce font-list churn.

On Mac OS, we should disable the notification observer for the kCTFontManagerRegisteredFontsChangedNotification while doing this, otherwise I think that will be triggered by the deactivation/activation operations, leading to a fresh UpdateFontList. Seems like this could easily turn into an infinite feedback loop whereby a change to the fonts triggers an update of our list which triggers a change to the fonts which triggers an update of our list.......

::: gfx/thebes/gfxFcPlatformFontList.h:319
(Diff revision 3)
>      gfxFontFamily* CreateFontFamily(const nsAString& aName) const override;
>  
>  #ifdef MOZ_BUNDLED_FONTS
>      void ActivateBundledFonts();
> +    void DeactivateBundledFonts();
>      nsCString mBundledFontsPath;

Actually, while you're touching this, there's no reason to save mBundledFontsPath in a member variable at all, is there? AFAICS, it's just used within ActivateBundledFonts, but once that method returns, nobody needs it any more. So it could be a local (stack) variable there, I think.

::: gfx/thebes/gfxGDIFontList.cpp:683
(Diff revision 3)
> +#ifdef MOZ_BUNDLED_FONTS
> +    ActivateBundledFonts();
> +#endif

There's no reason to have two separate #ifdef blocks here, is there? The Deactivate and Activate calls can go together.

In fact, I think I'd be inclined to get rid of the separate Deactivate methods, and just make each ActivateBundledFonts method start by cleaning up any list of previously-activated fonts that the platformFontList class has recorded.

::: gfx/thebes/gfxMacPlatformFontList.h:190
(Diff revision 3)
> -    void ActivateFontsFromDir(nsIFile* aDir);
> +    void ActivateFontsFromDir(nsIFile* aDir, bool aIsBundled = false);
>  
>  #ifdef MOZ_BUNDLED_FONTS
>      void ActivateBundledFonts();
> +    void DeactivateBundledFonts();
> +    nsTArray<nsCString> mBundledFontPaths;

If you make mBundledFontPaths a CFMutableArrayRef, you can stash references to the CFURLs there during ActivateBundledFonts, and then Deactivate will be much simpler -- it can just use the array directly, rather than having to recreate CFURLs and a CFArray from strings all over again.
Comment on attachment 8901693 [details]
Bug 1336208 - Part 1: Enable MOZ_BUNDLED_FONTS for MAC platform.

https://reviewboard.mozilla.org/r/173118/#review193102
Attachment #8901693 - Flags: review?(jfkthame) → review+
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review193104

I can't really review the JS code here in any detail -- that's way outside my area of knowledge -- but I'm feeling some concern about potential failure modes with this feature. Suppose, for example, there are new versions of some of the font resources. When launched, the browser is using the already-downloaded (old) versions, but it sees there are new versions and so it starts fetching them.

If I'm understanding the code here, each new font file will be moved into the bundled-fonts directory as it gets downloaded, replacing an old version. But Gecko won't know that has happened until all the downloads are successfully completed, and the download-fonts-finish notification is sent and processed. That means that there's an interval during which the actual files in the fonts directory have been modified underneath the running Gecko process, but it may still be trying to use the old versions (e.g. it may have cached some tables from the old font versions, but then if it needs to read something more from the file, it will be either unavailable or wrong, depending how the filesystem deals with the file having been overwritten).

Perhaps worse, if there's a failure during the font download, we may be in a situation where -some- of the font files have been replaced by new versions, but others are still old, and because the update was incomplete, the finish notification was never sent and Gecko wasn't told of the changes. So an inconsistent state -- font files that Gecko activated at startup have been changed behind its back -- could persist indefinitely (until the end of the session).

Am I misunderstanding something here, whereby this risk doesn't really exist, or do we need to take a more cautious approach? ISTM that we really need to be able to atomically install a set of new font files and tell Gecko to update its font list, not asynchronously replace individual files and then (some indeterminate time later) tell the process that was already using them about the change. The latter would be OK for -adding- new fonts, but it strikes me as dangerous for -updates-.
Another question I meant to ask: these fonts are being installed under the GRE directory, right? If Firefox is running without admin privileges, and is installed in a standard place such as the /Applications directory on macOS, will we actually be able to add/modify files there, or is it liable to be read-only for us?
Given that changes to the list of bundled fonts will be rare, I wonder if it would be better (safer, simpler) to drop the idea of on-the-fly updates altogether? Instead, the font-downloading code could put the resources into a separate fonts-staging directory, with no effect whatsoever on the running browser session. Then during the next startup, we'd check whether fonts-staging exists (or is non-empty), and if so we'd move the resources to the "real" fonts directory -before- initializing the font list.

If desired, we could inform the user when a new font collection has been downloaded to the staging area, e.g. with a simple infobar like "[x] The Firefox fonts collection has been updated. Restart your browser to use the new fonts" (without forcing the user to do so right away, obviously). Or we can just silently do the update whenever the restart happens.
(In reply to Jonathan Kew (:jfkthame) from comment #81)
> Another question I meant to ask: these fonts are being installed under the
> GRE directory, right? If Firefox is running without admin privileges, and is
> installed in a standard place such as the /Applications directory on macOS,
> will we actually be able to add/modify files there, or is it liable to be
> read-only for us?

I have checked my installation of Nightly on macOS, it turns out that the owner of the GRE folder is my user account. So, I think we can write data into the folder without admin privileges. Thanks for pointing this out.
(In reply to Jonathan Kew (:jfkthame) from comment #82)
> Given that changes to the list of bundled fonts will be rare, I wonder if it
> would be better (safer, simpler) to drop the idea of on-the-fly updates
> altogether? Instead, the font-downloading code could put the resources into
> a separate fonts-staging directory, with no effect whatsoever on the running
> browser session. Then during the next startup, we'd check whether
> fonts-staging exists (or is non-empty), and if so we'd move the resources to
> the "real" fonts directory -before- initializing the font list.
> 

I like this idea that this keeps things simpler and definitely safer. But, would this be harmful to performance during startup since it needs moving files. Especially for the first download because of it needs moving all fonts. One easy approach for resolving this is that we can directly move the download fonts into GRE folder if they are new added fonts, but we still leave the font update to the next startup. For those fonts who has old version in the GRE folder, we put them to the staging folder and move them at next startup as you suggested. 

> If desired, we could inform the user when a new font collection has been
> downloaded to the staging area, e.g. with a simple infobar like "[x] The
> Firefox fonts collection has been updated. Restart your browser to use the
> new fonts" (without forcing the user to do so right away, obviously). Or we
> can just silently do the update whenever the restart happens.
(In reply to Tim Huang[:timhuang] from comment #83)
> (In reply to Jonathan Kew (:jfkthame) from comment #81)
> > Another question I meant to ask: these fonts are being installed under the
> > GRE directory, right? If Firefox is running without admin privileges, and is
> > installed in a standard place such as the /Applications directory on macOS,
> > will we actually be able to add/modify files there, or is it liable to be
> > read-only for us?
> 
> I have checked my installation of Nightly on macOS, it turns out that the
> owner of the GRE folder is my user account. So, I think we can write data
> into the folder without admin privileges. Thanks for pointing this out.

Does it make a difference if Firefox is installed by one user (say, an administrator) but then routinely run by a different user without admin privileges?

The same question would apply to other operating systems, too -- will all Windows users be able to write data to the GRE directory, for example? Or do we need a separate process running with higher privileges to move the fonts into place?


(In reply to Tim Huang[:timhuang] from comment #84)

> would this be harmful to performance during startup since it needs moving
> files. Especially for the first download because of it needs moving all
> fonts.

There would be some cost at startup, but only when new fonts have been downloaded, so it would be a rare event. As such, I'm not much worried about it. It won't apply on first run of a newly-installed browser (because at that time we haven't downloaded the fonts yet), so the only expensive case is the first restart after a user enables anti-fingerprinting and we fetch all the fonts. Even then, moving a few dozen files within the local filesystem shouldn't take more than a few milliseconds, I guess.

Downloading new fonts directly into the GRE/fonts directory raises the issue of how incomplete font collections will behave -- if the user enables anti-fingerprinting, but then restarts the browser before all the fonts have been downloaded, we'll activate a partial collection of fonts on the next startup. Maybe this is OK, but I'm not entirely comfortable with the idea; ISTM it might be preferable to install the collection as a whole once all the files are available, rather than having a font collection that potentially changes on each startup, as extra resources are fetched.
(In reply to Jonathan Kew (:jfkthame) from comment #85)
> Does it make a difference if Firefox is installed by one user (say, an
> administrator) but then routinely run by a different user without admin
> privileges?
> 

I haven't tried this, but I think it needs admin privileges in this case. 

> The same question would apply to other operating systems, too -- will all
> Windows users be able to write data to the GRE directory, for example? Or do
> we need a separate process running with higher privileges to move the fonts
> into place?
> 

I am wondering that could we use a different directory other than font folder in GRE directory to hold downloaded fonts. For example, a folder inside the profile directory which doesn't have a privilege issue. The font list will register fonts from this folder as well as the font folder in GRE directory. But, I can still see some problems if the same font exists in both folders. One solution for this is that we can skip fonts that already exist in the GRE folder, so there will be no duplicates in both folder. However, the same issue comes when considering the same font with different versions. I don't know what will happen if we register both fonts in the system. Will the system use the newer version or it just replies an error? 

Jonathan, what do you think regarding this? Is this a viable approach?
Flags: needinfo?(jfkthame)
Using a fonts folder inside the profile dir is certainly possible, though it does come with some disadvantages: they won't survive creating a new profile, for example, and will need to be re-downloaded. Or on a machine where multiple users (each with their own profile) all use the same installation of Firefox, they'll each download and store separate copies of the fonts.

The question of different versions of the same font in the two directories is more of a problem. I don't think any of the systems we run on give any specific guarantee as to which font would get used if we register two (potentially different) copies of the "same" font (as identified by the internal font names, not filenames). In practice I suspect that in most cases the last one registered -- not necessarily the newest version of the font -- would "win", but we shouldn't rely on something like that. So I think we'd need to explicitly resolve any such conflicts and make sure we activate only the copy that we actually want to use. Which sounds potentially tricky/a bit expensive at startup, if we have to examine all the resources in both directories and resolve any duplicates (which means maintaining and checking version information of some kind for each of them) before we can decide what to activate.

An alternative might be that if we find new fonts to download, and can't install them into the GRE directory, we copy the current GRE fonts to the profile, and from then on, this profile only uses and maintains its local copies, and ignores the fonts in the GRE dir. So any given browser session will use downloaded fonts from only one location (avoiding issues with conflicting versions) -- either the GRE dir (in the usual case where this can be updated), so that all users/profiles share the resources, or the profile dir (in the case where the user may not have privileges to maintain the fonts under GRE).
Flags: needinfo?(jfkthame)
Attachment #8907447 - Flags: review?(wezhou) → review+
Hi Jonathan,

I have updated patches according to your comments. Could you please review them for me? Thanks.
Flags: needinfo?(jfkthame)
Hi Smaug,

Could you review the JS part of 'Part 5' patch? The js code, FontsDownloader.js, in this patch is responsible for downloading fonts from the server and put them into the staging directory in the profile directory. These fonts in the staging directory will be moved into the font directory at the next time when Firefox start-up.

Thanks,
Flags: needinfo?(bugs)
I'm definitely not the right person to review that. Some more js-savvy dev should take a look.
Flags: needinfo?(bugs)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review207388


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/components/resistfingerprinting/nsRFPService.cpp:442
(Diff revision 5)
> +
> +  rv = fontDir->Exists(&exist);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool isDirectory = false;
> +  rv = fontDir->IsDirectory(&isDirectory);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Flags: needinfo?(arthuredelstein)
Hi Gijs,

Could you review the JS part of patch 'Part 5'? I have put some descriptions in comment 96.

Thanks,
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review207794

::: modules/libpref/init/all.js:2596
(Diff revision 6)
>  
>  // Fonts downloading for fingerprinting resistance via settings server (Kinto)
>  pref("privacy.resistFingerprinting.fonts.bucket", "fingerprinting-defenses");
>  pref("privacy.resistFingerprinting.fonts.collection", "fonts");
>  pref("privacy.resistFingerprinting.fonts.checked", 0);
> +pref("privacy.resistFingerprinting.fonts.server", "https://net-mozaws-stage-kinto-fennec.s3.amazonaws.com/");

This URL looks wrong for several reasons:
- this is all.js but the URL contains "fennec"
- the URL contains "stage"
- the controlling domain is amazonaws.com.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:39
(Diff revision 6)
> +async function fetchAndConvert(aURI, aOriginType, aTargetType) {
> +  return new Promise((resolve, reject) => {

Given that there are several things in here that could theoretically throw (that you're manually rejecting for), you could just only put the actual converter in the promise method, as nothing else will need access to reject/resolve.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:45
(Diff revision 6)
> +  return new Promise((resolve, reject) => {
> +    let channel = NetUtil.newChannel({
> +      uri: aURI,
> +      loadUsingSystemPrincipal: true,
> +      securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
> +      contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER

I don't think we should need to use this. Maybe check with :ckerschb ?

::: toolkit/components/resistfingerprinting/FontsDownloader.js:48
(Diff revision 6)
> +    if (!channel) {
> +      reject(new Error("Error when creating channel for fetching and converting."));
> +    }

This would throw (see other instances below).

::: toolkit/components/resistfingerprinting/FontsDownloader.js:55
(Diff revision 6)
> +    if (!convertService) {
> +      reject(new Error("Error when creating converter service."));
> +    }

This can't happen in JS - the getService call would throw if the result wasn't OK.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:58
(Diff revision 6)
> +
> +    let converter = convertService.asyncConvertData(aOriginType, aTargetType, {
> +      data: [],
> +      onStartRequest(aR, aC) {},
> +      onDataAvailable(aR, aC, aStream, aO, aCount) {
> +        let binaryStream = Cc["@mozilla.org/binaryinputstream;1"].
> +                             createInstance(Ci.nsIBinaryInputStream);
> +        binaryStream.setInputStream(aStream);
> +        let input = binaryStream.readByteArray(aCount);
> +        this.data.push(...input);
> +      },
> +      onStopRequest(aR, aC, aStatusCode) {
> +        if (!Components.isSuccessCode(aStatusCode)) {
> +          reject(new Error("Error when fetching and converting data."));
> +        }
> +
> +        resolve(new Uint8Array(this.data));
> +      }
> +    }, null);

This loads the entire contents of the stream into memory twice, which seems unfortunate, and uses a bunch of XPCOM to get a Uint8Array. Why not just use fetch(), or XHR ? I see you noted in other comments here that the server doesn't send the right content-encoding header. Can't we fix that - don't we control kinto ourselves? And wouldn't we want the font files to live somewhere more CDN-y than kinto?

If we really really really can't fix the server, I would almost suggest that it would be better to fix the response headers by using xhr.channel (QI to nsIHttpChannel, call setResponseHeader from an onStartRequest listener)

Also, no error handling...

::: toolkit/components/resistfingerprinting/FontsDownloader.js:78
(Diff revision 6)
> +    if (!converter) {
> +      reject(new Error("Error when creating converter."));
> +    }

Again, this would have thrown, so you don't need to nullcheck.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:134
(Diff revision 6)
> +      // We notify the observer for telling fonts have been downloaded successfully.
> +      // This is for testing purpose.
> +      Services.obs.notifyObservers(null, "resist-fingerprinting:download-fonts-success");
> +    } catch (aError) {
> +      Cu.reportError(aError);
> +      Services.obs.notifyObservers(null, "resist-fingerprinting:download-fonts-failure");

What listens for these failure/success messages? Why do they need broadcasting? I don't see any telemetry in this patch, and even if there was, how likely is that to be turned on for users of anti-fingerprinting/tor ?

::: toolkit/components/resistfingerprinting/FontsDownloader.js:144
(Diff revision 6)
> +    // We first check the existence of fonts list file in profile directory.
> +    // This file is the up-to-date list which is updated by the blocklist client.
> +    let fontsListFile = FileUtils.getFile(
> +      "ProfD", [RFP_FONTS_LIST_DIR, RFP_FONTS_LIST_FILE], true);
> +
> +    if (!fontsListFile.exists()) {

This is main-thread IO, please use `OS.File` instead. But really, AIUI the correct way to do this would be to just call `OS.File.read` and handle failure, instead of first doing a "does this file exist" check.

Same lower down. Doing this 'right' would also help in case the file does exist, but the OS doesn't let you read it for some reason - then we'd still be falling back to the other file, which seems like it would be the right behaviour (instead of assuming that the file existing must mean that we can read it etc.)

::: toolkit/components/resistfingerprinting/FontsDownloader.js:163
(Diff revision 6)
> +
> +    return fontsList;
> +  },
> +
> +  async downloadFonts(aFontList) {
> +    let fonts = aFontList["data"];

Nit: Use `.filter(f => f.platforms.includes(AppConstants.platform));` here rather than indenting the entire contents of the for loop below. In fact, might use this to immediately filter out the 'already updated' ones, too.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:179
(Diff revision 6)
> +        let fontFilename = font.attachment.original.filename;
> +        let localFontFile = this._getLocalFontFile(fontFilename);
> +
> +        try {
> +          // Check the existence of local font file and verify whether it is up-to-date.
> +          if (localFontFile.exists()) {

More main-thread IO, plus you're calling .read() anyway, so you might as well just do that and see what fails.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:189
(Diff revision 6)
> +          let fontURL = Services.prefs.getCharPref(RFP_FONTS_DOWNLOAD_SERVER_PREF) +
> +                        font.attachment.location;
> +          let fontData = await fetchAndConvert(NetUtil.newURI(fontURL), "gzip", "uncompressed");

For N fonts, now we call newURI N times. That's wrong - this code should live outside the loop. Plus, there's no point running through the loop if the server URI is not valid, we should bail out sooner (and we should probably make sure to not throw an error in this case, so that it's possible to disable this code with a hotfix by clearing the pref, without then causing errors on every startup).

::: toolkit/components/resistfingerprinting/FontsDownloader.js:199
(Diff revision 6)
> +          if (!await verifyHash(fontData, font.attachment.original.hash)) {
> +            continue;
> +          }

I don't really understand the hashing here. Why do we do it? My understanding is:

- the fonts list is user-writable
- the font is user-writable
- the font list comes from the internet (besides the one we ship)
- the fonts come from the internet

I don't see a threat model where the hashing buys us anything. If any of the data is compromised, the rest of the data would just as easily also be compromised.

If we're just checking integrity rather than dealing with security, we shouldn't use a sha256 hash, but something significantly cheaper, like crc (which you can get from just OS.File, IIRC).

If there's a reason we must continue to use crypto hashes, then this should be documented, and ideally the hash shouldn't be hardcoded to a specific hashing algorithm.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:203
(Diff revision 6)
> +          let stagingFontFile = FileUtils.getFile(
> +            "ProfD", [RFP_FONTS_STAGING_DIR, fontFilename], true);

Instead of the repeated use of FileUtils in this file, please use OS.Path.join() which is likely cheaper because it doesn't involve several XPConnect roundtrips, the creation of an nsIFile instance that we then destroy immediately, and saves us from importing FileUtils. 

This would also help because it's easier to save an intermediate result, like the staging dir, and only append the font file name, instead of reconstructing those bits of the path every time we go through the loop.

::: toolkit/components/resistfingerprinting/FontsDownloader.js:210
(Diff revision 6)
> +        } catch (e) {
> +          throw e;
> +        }

This is a useless try...catch block... :-\

::: toolkit/components/resistfingerprinting/FontsDownloader.js:222
(Diff revision 6)
> +  uninit() {
> +    Services.obs.removeObserver(this, "xpcom-shutdown");
> +    Services.obs.removeObserver(this, "resist-fingerprinting:start-download-fonts");
> +  },

Looks like you only use uninit to unregister the observer? That shouldn't be necessary. On the other hand, shouldn't we abort in-progress downloads at that point?

::: toolkit/components/resistfingerprinting/FontsDownloader.js:227
(Diff revision 6)
> +  _getLocalFontFile(aFontFileName) {
> +    let localFontFile = FileUtils.getFile("ProfD", [RFP_FONTS_LIST_DIR], false);
> +
> +    // We use the font file in profile directory if it exists. Otherwise, we use
> +    // the one in GRE directory.
> +    if (!localFontFile.exists()) {
> +      localFontFile = FileUtils.getFile("GreD", [RFP_FONTS_LIST_DIR], false);
> +    }
> +
> +    localFontFile.append(aFontFileName);
> +    return localFontFile;
> +  }

There's no documentation for this method, but it does the opposite of the C++ RFP service (which prefers GreD). Why?

::: toolkit/components/resistfingerprinting/FontsDownloader.manifest:3
(Diff revision 6)
> +component {c2987238-9a4f-4461-a11a-ca4bd13f11eb} FontsDownloader.js process=main
> +contract @mozilla.org/fontsDownloader;1 {c2987238-9a4f-4461-a11a-ca4bd13f11eb} process=main
> +category profile-after-change FontsDownloader @mozilla.org/fontsDownloader;1 process=main

Does this really need to happen this early in startup? Seems to me like we could initialize this later, like by just instantiating the service lazily from somewhere like nsBrowserGlue, well after startup.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:400
(Diff revision 6)
> +  rv = fontDir->Exists(&exist);
> +  NS_ENSURE_SUCCESS(rv, rv);

This looks like main-thread IO, from the assertion in MaybeDownloadFonts. We should not be adding more main-thread IO...
Attachment #8901697 - Flags: review-
For future reference, you can modify the list of reviewers via the reviewboard user interface by clicking the little pencil icon next to the list of reviewers of a given cset.
Flags: needinfo?(gijskruitbosch+bugs)
Hi Jason,

Would be possible to add a Content-Encoding header for gzipped attachments in Kinto Server? By doing so, fetch and xhr can decode gzipped attachments transparently.

Thanks,
Flags: needinfo?(jthomas)
Comment on attachment 8901698 [details]
Bug 1336208 - Part 6: Apply the font whitelist when pref 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/173128/#review208198

A few questions on this, so clearing r? for now... please re-request in due course.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:67
(Diff revision 6)
>  // A message for telling nsRFPService to move fonts, this is for testing purpose.
>  #define RFP_MOVE_FONTS_MSG "resist-fingerprinting:move-fonts"
>  // A message for notifying the moving of fonts is finished, this is for testing purpose.
>  #define RFP_MOVE_FONTS_FINISH "resist-fingerprinting:move-fonts-finish"
>  
> +// The font whitelist.

Doing this as hard-coded strings in C++ seems really ugly. Can't we store these lists in a preference?

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:81
(Diff revision 6)
> +  Noto Sans Myanmar, Noto Sans Yi, Nyala, PMingLiU, \u65b0\u7d30\u660e\u9ad4, Plantagenet Cherokee, \
> +  Raavi, Segoe UI, Shruti, SimSun, \u5b8b\u4f53, Sylfaen, Tahoma, Times New Roman, Tunga, Verdana, \
> +  Vrinda, Yu Gothic UI"
> +#elif XP_MACOSX
> +#define RFP_FONTS_WHITELIST "AppleGothic, Apple Color Emoji, Arial, Courier, Geneva, \
> +  Georgia, Heiti TC, Helvetica, Helvetica Neue, .Helvetica Neue DeskInterface, \

I don't think .Helvetica Neue DeskInterface belongs in this list. A dot-prefixed font family name is a "hidden" system font that is not exposed directly by name at all; it's only used via OS theme APIs for drawing interface elements. And for that purpose (drawing UI), the whitelist doesn't apply anyhow.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:92
(Diff revision 6)
> +#define RFP_FONTS_WHITELIST "Arimo Regular, Arimo Bold, Arimo Italic, Arimo Bold Italic, \
> +  Cousine, Tinos, Tinos Bold, Tinos Italic, Tinos Bold Italic, Noto Naskh Arabic, Noto Sans Armenian, \

The whitelist is (supposed to be) a list of font family names, so it shouldn't be necessary (or even correct at all) to list individual faces like "Arimo Regular", "Arimo Bold", etc.; the single family name "Arimo" should be all that's needed. Likewise for Tinos. Unless the fonts aren't correctly organized as families -- in which case that should be fixed!

(Why these particular fonts, anyhow? Obviously (from the names), Arial and Times clones, but they're not the most commonly used default sans and serif families on Linux systems, AFAIK. Wouldn't something like DejaVu or FreeSans/Serif or even Linux Libertine be more typical?)

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:620
(Diff revision 6)
> +  nsCOMPtr<nsIFontEnumerator> fontEnum = do_GetService("@mozilla.org/gfx/fontenumerator;1", &rv);
> +
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return;
> +  }
> +
> +  bool unused;
> +
> +  // This will update the global font list and apply the font whitelist when
> +  // the pref 'privacy.resistFingerprinting' is true. When it is false, this
> +  // will clear the font whitelist.
> +  fontEnum->UpdateFontList(&unused);

I think it should be sufficient (and simpler) to just call gfxPlatform::UpdateFontList() here. Or is there some reason that wouldn't work?
Attachment #8901698 - Flags: review?(jfkthame)
Comment on attachment 8901694 [details]
Bug 1336208 - Part 2: Making the gfxPlatformFontList to use fonts directory in profile folder if it exists for bundled fonts.

https://reviewboard.mozilla.org/r/173120/#review208202

This generally looks OK, but I wonder if we should use a different subdir name than just "fonts"? On Android, we already support loading fonts from a "fonts" directory in the profile, which is intended for fonts downloaded/added by user choice (e.g. an add-on), which is rather different from the RFP font package. I realize there isn't currently a RFP whitelist for Android in these patches, but presumably there might be one day? And ISTM that with user-added fonts and RFP-whitelist fonts being two very different categories, we probably should keep them separate -- which suggests the $profile/fonts directory, at least on Android, won't be the right place to use.

So I wonder, should we use a more specific directory name such as $profile/rfp-fonts from the beginning, to avoid potential confusion in the future?
Attachment #8901694 - Flags: review?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #107)
> Comment on attachment 8901694 [details]
> Bug 1336208 - Part 2: Making the gfxPlatformFontList to use fonts directory
> in profile folder if it exists for bundled fonts.
> 
> https://reviewboard.mozilla.org/r/173120/#review208202
> 
> This generally looks OK, but I wonder if we should use a different subdir
> name than just "fonts"? On Android, we already support loading fonts from a
> "fonts" directory in the profile, which is intended for fonts
> downloaded/added by user choice (e.g. an add-on), which is rather different
> from the RFP font package. I realize there isn't currently a RFP whitelist
> for Android in these patches, but presumably there might be one day? And
> ISTM that with user-added fonts and RFP-whitelist fonts being two very
> different categories, we probably should keep them separate -- which
> suggests the $profile/fonts directory, at least on Android, won't be the
> right place to use.
> 
> So I wonder, should we use a more specific directory name such as
> $profile/rfp-fonts from the beginning, to avoid potential confusion in the
> future?

This idea sounds good to me. I agree that we should use a different directory here. However, I have a small question. Should we only use profile directory here or should we use the previous approach, using GRE directory first or using profile directory if the GRE directory is not writable. To me, using profile directory solely seems much simpler since we don't need to test the GRE directory. But, it requires download for every profile.

Jonathan, what do you think about this?
(In reply to Tim Huang[:timhuang] from comment #105)
> Hi Jason,
> 
> Would be possible to add a Content-Encoding header for gzipped attachments
> in Kinto Server? By doing so, fetch and xhr can decode gzipped attachments
> transparently.
> 
> Thanks,

I believe this would require a update to https://github.com/Kinto/kinto-attachment, adding needinfo :natim
Flags: needinfo?(jthomas) → needinfo?(rhubscher)
> Would be possible to add a Content-Encoding header for gzipped attachments
> in Kinto Server? By doing so, fetch and xhr can decode gzipped attachments
> transparently.

sebastian do you recall why we didn't do it like that for Fennec fonts?
Flags: needinfo?(rhubscher) → needinfo?(s.kaspari)
> sebastian do you recall why we didn't do it like that for Fennec fonts?

Because this will cause the http client of Android to decompress the stream on-the-fly. Which means we can't support resuming downloads (because we have no idea what the correct byte offset is).

https://bugzilla.mozilla.org/show_bug.cgi?id=1212353#c23
Flags: needinfo?(s.kaspari)
(In reply to Tim Huang[:timhuang] from comment #108)
> This idea sounds good to me. I agree that we should use a different
> directory here. However, I have a small question. Should we only use profile
> directory here or should we use the previous approach, using GRE directory
> first or using profile directory if the GRE directory is not writable. To
> me, using profile directory solely seems much simpler since we don't need to
> test the GRE directory. But, it requires download for every profile.
> 
> Jonathan, what do you think about this?

I'm not sure; on the one hand, it could simplify the code quite a bit, but on the other hand, there's a cost in added disk and bandwidth usage for the multiple downloads in the case where more than one profile using the same Firefox install requests anti-fingerprinting.

Using the GRE (when possible) also has the benefit that if one user (profile) has requested RFP, and the fonts have been downloaded, they'll be available immediately to any subsequent profile that also requests it, rather than those profiles turning on the feature but not actually having the fonts until their own separate downloads are complete.

(Do we in some way let the user know when this is the case? I.e. they've "enabled" RFP, but we're waiting for fonts and so the whitelist is not yet in effect.... does the user know that fingerprint-resistance is not yet fully operational?)

Whether the simplification of only using the profile dir is worth the added cost of duplicating the downloads seems to me like a higher-level product decision, not really a technical issue. Can we get a measurement of the total amount of data we're talking about (for each platform) here?
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #112)
> I'm not sure; on the one hand, it could simplify the code quite a bit, but
> on the other hand, there's a cost in added disk and bandwidth usage for the
> multiple downloads in the case where more than one profile using the same
> Firefox install requests anti-fingerprinting.
> 
> Using the GRE (when possible) also has the benefit that if one user
> (profile) has requested RFP, and the fonts have been downloaded, they'll be
> available immediately to any subsequent profile that also requests it,
> rather than those profiles turning on the feature but not actually having
> the fonts until their own separate downloads are complete.
> 
> (Do we in some way let the user know when this is the case? I.e. they've
> "enabled" RFP, but we're waiting for fonts and so the whitelist is not yet
> in effect.... does the user know that fingerprint-resistance is not yet
> fully operational?)
> 

Right now, we don't have a user interface for this purpose. I think it would be good if we have an interface to inform users regarding their protection of fingerprinting resistance. However, it requires an elegant UX design, so it should be handled in a separated bug.

> Whether the simplification of only using the profile dir is worth the added
> cost of duplicating the downloads seems to me like a higher-level product
> decision, not really a technical issue. Can we get a measurement of the
> total amount of data we're talking about (for each platform) here?

The amount of data for each platform as follows:
  Linux: 24 MB for downloading and 32 MB for storage.
  Mac: 1.7 MB for downloading and 3.4 MB for storage.
  Win: 197 KB for downloading and 197 KB for storage.

Given the amount of data for downloading above, especially for Linux, I think using the GRE directory if we can is a more practical approach than only using profile direcotry. And subsequent profiles can have the protection at very first startup is a strong advantage.

Jonathan, What do you think?
Flags: needinfo?(jfkthame)
Yeah, tens of MB seems a lot to potentially be adding to every profile (and downloading repeatedly). So it's probably worth trying to store the fonts under the GRE if possible.

Should we somehow leverage the updater to do the work here, rather than the regular firefox process? Presumably we run the updater with elevated permissions that allow it to modify the installed browser, so it is more likely to be able to modify resources within the GRE directory -- right? (I don't really know anything about it...)
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #114)
> Yeah, tens of MB seems a lot to potentially be adding to every profile (and
> downloading repeatedly). So it's probably worth trying to store the fonts
> under the GRE if possible.
> 
> Should we somehow leverage the updater to do the work here, rather than the
> regular firefox process? Presumably we run the updater with elevated
> permissions that allow it to modify the installed browser, so it is more
> likely to be able to modify resources within the GRE directory -- right? (I
> don't really know anything about it...)

It would be wonderful if the updater is capable of moving files into GRE directory. But, I am afraid of this is not the case for every platform. See [1], it looks like the updater in Linux cannot write into installation directory if it doesn't have a proper permission. But, maybe I am wrong about this. 

Matt, could you provide us with your insights regarding this? Thanks.


[1] https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#402
Flags: needinfo?(mhowell)
The updater doesn't really support manipulating files outside of actually installing an update, so if you want to use the updater for this then just including the fonts with Firefox would be the only option there (I admit I haven't read the whole history on this bug, it's very long, but I assume that's not desirable). Also, you're correct about Linux; on that platform, we don't have a way to give the updater elevated permissions beyond those of the user running the browser, plus in the likely case that the browser was installed from a package provided by the Linux distribution, then our updater is probably disabled anyway.

There is precedent for downloading optional files like this into the profile directory, because that's how GMP plugins are handled, and those can be several MB each (my profile on Windows has three of those, totaling 17 MB). Using the profile directory here isn't ideal, but I'm not sure it's a major problem either.
Flags: needinfo?(mhowell)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review207794

> What listens for these failure/success messages? Why do they need broadcasting? I don't see any telemetry in this patch, and even if there was, how likely is that to be turned on for users of anti-fingerprinting/tor ?

The audience of these two events is the test case which will be added in Part 7. The testing code needs to listen them to know whether the download is succeeded and perform following checks.

> I don't really understand the hashing here. Why do we do it? My understanding is:
> 
> - the fonts list is user-writable
> - the font is user-writable
> - the font list comes from the internet (besides the one we ship)
> - the fonts come from the internet
> 
> I don't see a threat model where the hashing buys us anything. If any of the data is compromised, the rest of the data would just as easily also be compromised.
> 
> If we're just checking integrity rather than dealing with security, we shouldn't use a sha256 hash, but something significantly cheaper, like crc (which you can get from just OS.File, IIRC).
> 
> If there's a reason we must continue to use crypto hashes, then this should be documented, and ideally the hash shouldn't be hardcoded to a specific hashing algorithm.

I agree with you that this hashing is kind of redundent. Thanks. I will remove this.

> Looks like you only use uninit to unregister the observer? That shouldn't be necessary. On the other hand, shouldn't we abort in-progress downloads at that point?

Yes, we need to abort downloading. I will fix this.

> There's no documentation for this method, but it does the opposite of the C++ RFP service (which prefers GreD). Why?

We prefer to put the fonts in the GRE directory since GRE directory is shared with all profiles. By doing so, we only have to download once then all profiles can use them. However, the profile of a regular user account cannot write into GRE directory if Firefox is installed by a privileged account or by another user account. In this case, we will download fonts into the profile's directory and fonts will be maintained in there for this profile. 

Therefore, we will first check the existence of fonts in the profile directory when we want to access the downloaded fonts.
(In reply to Jonathan Kew (:jfkthame) from comment #106)
> Comment on attachment 8901698 [details]
> Bug 1336208 - Part 6: Apply the font whitelist when pref
> 'privacy.resistFingerprinting' is true.
> 
> https://reviewboard.mozilla.org/r/173128/#review208198
> 
> ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:92
> (Diff revision 6)
> > +#define RFP_FONTS_WHITELIST "Arimo Regular, Arimo Bold, Arimo Italic, Arimo Bold Italic, \
> > +  Cousine, Tinos, Tinos Bold, Tinos Italic, Tinos Bold Italic, Noto Naskh Arabic, Noto Sans Armenian, \
> 
> The whitelist is (supposed to be) a list of font family names, so it
> shouldn't be necessary (or even correct at all) to list individual faces
> like "Arimo Regular", "Arimo Bold", etc.; the single family name "Arimo"
> should be all that's needed. Likewise for Tinos. Unless the fonts aren't
> correctly organized as families -- in which case that should be fixed!
> 
> (Why these particular fonts, anyhow? Obviously (from the names), Arial and
> Times clones, but they're not the most commonly used default sans and serif
> families on Linux systems, AFAIK. Wouldn't something like DejaVu or
> FreeSans/Serif or even Linux Libertine be more typical?)
> 

Arthur, could you answer this for me?
(In reply to Tim Huang[:timhuang] (OOO: 12/6 ~ 12/8) from comment #117)
> > There's no documentation for this method, but it does the opposite of the C++ RFP service (which prefers GreD). Why?
> 
> We prefer to put the fonts in the GRE directory since GRE directory is
> shared with all profiles. By doing so, we only have to download once then
> all profiles can use them. However, the profile of a regular user account
> cannot write into GRE directory if Firefox is installed by a privileged
> account or by another user account. In this case, we will download fonts
> into the profile's directory and fonts will be maintained in there for this
> profile. 
> 
> Therefore, we will first check the existence of fonts in the profile
> directory when we want to access the downloaded fonts.

I don't follow the "therefore" here. If we prefer GreD when putting fonts in places, why shouldn't we check that place first in the JS code? Does the JS code never run if the fonts were successfully put in the GreD?

Fun things: what happens if:

1) user A runs Firefox and installs fonts in GreD
2) user B runs Firefox and realizes fonts can (should?) be updated, but has no write access to GreD
Flags: needinfo?(tihuang)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review211084

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:40
(Diff revision 7)
> + * @param {Uint8Array} aData      The data to be verified.
> + * @param {String}     aHash      The hash value
> + * @param {String}     aAlgorithm The hash algorithm
> + * @return {boolean} A boolean indicates whether hash matches.
> + */
> +async function verifyHash(aData, aHash, aAlgorithm) {

I'm a bit confused because you said you'd get rid of the hashing, but it's still here?

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:96
(Diff revision 7)
> +      Services.obs.notifyObservers(null, "resist-fingerprinting:download-fonts-success");
> +    } catch (aError) {
> +      Cu.reportError(aError);
> +      Services.obs.notifyObservers(null, "resist-fingerprinting:download-fonts-failure");

So functionally, I don't really understand how these are necessary for tests. Why can't the unit tests just call this method and catch exceptions and/or assume success after the async method completes? That'd provide the same kind of guarantee...

Really, for success, a test should check that the correct files are installed and can be used. Otherwise we're only verifying that this code thinks it worked, not whether it actually did, in fact, work. And if we are already checking that the files get installed successfully, I don't think we need these observer notifications.

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:135
(Diff revision 7)
> +    let data = await OS.File.read(fontsListPath, {encoding: "utf-8"});
> +    return JSON.parse(data);
> +  },
> +
> +  async downloadFonts(aFontList) {
> +    let fonts = aFontList.data;

Why is everything wrapped in a "data" member? It seems like the JSON file could just contain only the array?

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:142
(Diff revision 7)
> +    // If the server url is empty, we will bail out here and throw nothing. So,
> +    // we can disable the whole download by clearing this server pref.
> +    let serverURL = Services.prefs.getCharPref(RFP_FONTS_DOWNLOAD_SERVER_PREF);
> +
> +    if (serverURL === "") {
> +      return;
> +    }

We should do this before filtering the list of fonts.

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:146
(Diff revision 7)
> +
> +    // If the server url is empty, we will bail out here and throw nothing. So,
> +    // we can disable the whole download by clearing this server pref.
> +    let serverURL = Services.prefs.getCharPref(RFP_FONTS_DOWNLOAD_SERVER_PREF);
> +
> +    if (serverURL === "") {

Nit: Firefox style guide asks for just:

```js
if (!serverURL)
```

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:162
(Diff revision 7)
> +
> +      // Check the local font file and verify whether it is up-to-date.
> +      try {
> +        let localFontData = await OS.File.read(localFontPath);
> +
> +        if (await verifyHash(localFontData, font.attachment.hash, "SHA-256")) {

If we do need to keep this, why is it in the try...catch?

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:217
(Diff revision 7)
> +  async _getLocalFontPath(aFontFileName) {
> +    // First, we try to get the path of font directory.
> +    if (!this._getLocalFontDirPath) {
> +      // We use the font file in profile directory if it exists. Otherwise, we use
> +      // the one in GRE directory.
> +      let localFontPath = OS.Path.join(OS.Constants.Path.profileDir,

Should we use `localProfileDir` rather than syncing stuff across roaming windows environments?

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:222
(Diff revision 7)
> +        let greDir = Services.dirsvc.get("GreD",
> +                                         Ci.nsIFile);

As noted in the bug, still not convinced that this needs to be the opposite order of the other code...

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:414
(Diff revision 7)
> +
> +  rv = destFontDir->Append(NS_LITERAL_STRING(RFP_FONT_DIR));
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +
> +  nsCOMPtr<nsISimpleEnumerator> entries;
> +  rv = stagingDir->GetDirectoryEntries(getter_AddRefs(entries));

Thank you for moving all of the JS IO to OS.File. Unfortunately it seems the C++ code is still using main-thread IO. Though I re-read the last comments on the bug, I haven't seen this addressed, either.


Can we move this off the main thread, like to a runnable, maybe? (I'm very much not a C++ IO expert, so please check with other folks that that suggestion actually makes things better here, but off-hand that would seem better than what this is currently doing.)
Attachment #8901697 - Flags: review?(gijskruitbosch+bugs) → review-
(In reply to :Gijs from comment #126)
> (In reply to Tim Huang[:timhuang] (OOO: 12/6 ~ 12/8) from comment #117)
> I don't follow the "therefore" here. If we prefer GreD when putting fonts in
> places, why shouldn't we check that place first in the JS code? Does the JS
> code never run if the fonts were successfully put in the GreD?
> 

Sorry, there is one thing which I didn't tell before is that the fonts in the 'rfp-fonts' directory of the GreD won't be loaded if the 'rfp-fonts' directory of the profile directory is present. In this case, Firefox will only load downloaded fonts in the profile directory. This approach was proposed in comment 87.

So, when we checking the existing local fonts for up-to-dateness, we will first check fonts in the profile directory to see whether this profile should use its own fonts. If it doesn't have to use its own fonts, the fonts in profile directory not exist, then it will go to check fonts in GreD.

Does this make sense to you?

> Fun things: what happens if:
> 
> 1) user A runs Firefox and installs fonts in GreD
> 2) user B runs Firefox and realizes fonts can (should?) be updated, but has
> no write access to GreD

Unfortunately, there will be one problem in this case for the currect patch I wrote; the fonts in the profile directory of the user B won't be completed. Sorry, I didn't think straight before. Followings are what going to happen when user B downloads fonts:
  1. The user B will check the up-to-dateness of fonts in GreD since the 'rfp-fonts' directory doesn't exist in its profile directory yet.
  2. The user B will figure out some fonts need to be updated and download them into its staging directory.
  3. At next startup of Firefox, only these new downloaded fonts are moved into 'rfp-fonts' directory in the profile directory. Those fonts that don't get updated won't be there. That's to say, the fonts are not completed for fingerprinting resistance.

One solution for this is that we can copy those fonts that are not updated into staging directory from GreD at step 2. So, the staging directory will contain all fonts we need.
Flags: needinfo?(tihuang) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review211084

> I'm a bit confused because you said you'd get rid of the hashing, but it's still here?

We still need this function for checking the up-to-dateness of the local fonts. I know that SHA-256 is expensive, but this is the only thing the server provides. The download will happen so rarely, checking font list happens once a day and the hash will only run when fonts get updated. The fonts stable for about two years for Tor browser and I think it will be similar situation for us. Given that, perhaps it will not drag performance too much.

> So functionally, I don't really understand how these are necessary for tests. Why can't the unit tests just call this method and catch exceptions and/or assume success after the async method completes? That'd provide the same kind of guarantee...
> 
> Really, for success, a test should check that the correct files are installed and can be used. Otherwise we're only verifying that this code thinks it worked, not whether it actually did, in fact, work. And if we are already checking that the files get installed successfully, I don't think we need these observer notifications.

Yes, you are right about this. I will fix this.

> Why is everything wrapped in a "data" member? It seems like the JSON file could just contain only the array?

This format is provided by the kinto server. The font list file here is reusing the data from the kinto server. So there is a 'data' member here.

> If we do need to keep this, why is it in the try...catch?

Yes, this should be move out. Thanks.

> Thank you for moving all of the JS IO to OS.File. Unfortunately it seems the C++ code is still using main-thread IO. Though I re-read the last comments on the bug, I haven't seen this addressed, either.
> 
> 
> Can we move this off the main thread, like to a runnable, maybe? (I'm very much not a C++ IO expert, so please check with other folks that that suggestion actually makes things better here, but off-hand that would seem better than what this is currently doing.)

I think we need to do this in the main thread since we need to move the fonts into the place before the initialization of gfxPlatformFontList, see comment 82. And replacing fonts after they got loaded might cause unexpected behaviors. The performance problem we had talked before in comment 84 and comment 85.
(In reply to Tim Huang[:timhuang] (OOO: 12/6 ~ 12/8) from comment #128)
> (In reply to :Gijs from comment #126)
> > (In reply to Tim Huang[:timhuang] (OOO: 12/6 ~ 12/8) from comment #117)
> > I don't follow the "therefore" here. If we prefer GreD when putting fonts in
> > places, why shouldn't we check that place first in the JS code? Does the JS
> > code never run if the fonts were successfully put in the GreD?
> > 
> 
> Sorry, there is one thing which I didn't tell before is that the fonts in
> the 'rfp-fonts' directory of the GreD won't be loaded if the 'rfp-fonts'
> directory of the profile directory is present. In this case, Firefox will
> only load downloaded fonts in the profile directory. This approach was
> proposed in comment 87.
> 
> So, when we checking the existing local fonts for up-to-dateness, we will
> first check fonts in the profile directory to see whether this profile
> should use its own fonts. If it doesn't have to use its own fonts, the fonts
> in profile directory not exist, then it will go to check fonts in GreD.
> 
> Does this make sense to you?

OK, yes, this makes more sense. Can you add some comments to this effect in the code? Maybe something like:

/**
 * Find the directory to use for rfp fonts. Registering fonts from 2 directories
 * is likely to cause issues. While we prefer the GreD directory in the application,
 * the user may not have write permissions for it. If this is the case, fonts will
 * be downloaded and/or copied into the profile directory. Thus, when looking for
 * fonts, we check the profile directory first, because if there are fonts there
 * they are guaranteed to be at least as new as what is in GreD.
 */


> > Fun things: what happens if:
> > 
> > 1) user A runs Firefox and installs fonts in GreD
> > 2) user B runs Firefox and realizes fonts can (should?) be updated, but has
> > no write access to GreD
> 
> Unfortunately, there will be one problem in this case for the currect patch
> I wrote; the fonts in the profile directory of the user B won't be
> completed. Sorry, I didn't think straight before. Followings are what going
> to happen when user B downloads fonts:
>   1. The user B will check the up-to-dateness of fonts in GreD since the
> 'rfp-fonts' directory doesn't exist in its profile directory yet.
>   2. The user B will figure out some fonts need to be updated and download
> them into its staging directory.
>   3. At next startup of Firefox, only these new downloaded fonts are moved
> into 'rfp-fonts' directory in the profile directory. Those fonts that don't
> get updated won't be there. That's to say, the fonts are not completed for
> fingerprinting resistance.
> 
> One solution for this is that we can copy those fonts that are not updated
> into staging directory from GreD at step 2. So, the staging directory will
> contain all fonts we need.

This sounds sensible to me.
(In reply to Tim Huang[:timhuang] (OOO: 12/6 ~ 12/8) from comment #129)
> Comment on attachment 8901697 [details]
> Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting
> resistance.
> 
> https://reviewboard.mozilla.org/r/173126/#review211084
> 
> > I'm a bit confused because you said you'd get rid of the hashing, but it's still here?
> 
> We still need this function for checking the up-to-dateness of the local
> fonts. I know that SHA-256 is expensive, but this is the only thing the
> server provides.

This is unfortunate. Does it not provide a file size, either, like we use to check things elsewhere? That seems like it'd be cheaper to check, and if that mismatches we know the hash will also mismatch and wouldn't need to read the entire (several MB...) file into memory and hash it...

> The download will happen so rarely, checking font list
> happens once a day and the hash will only run when fonts get updated. The
> fonts stable for about two years for Tor browser and I think it will be
> similar situation for us. Given that, perhaps it will not drag performance
> too much.

This is reassuring in terms of performance, thanks for explaining.

However, I'm looking at the code again, because of this... I'm a bit confused. As far as I can tell, we read a local font list file which contains "last modified" timestamps for the fonts. This file is downloaded regularly by the blocklist client, according to the comments in the patch - which I believe runs in-process, so only when Firefox runs.

When we download font files, they are moved from staging to their destination on the next startup. At *that* point, we set the pref for the last time we moved fonts.

Does this mean that in the following situation can occur:

1. start Firefox on timestamp X, update font list from blocklist, it contains fonts that are updated on Day X
2. close Firefox
3. on timestamp X + 5, we update the font list on the server
4. on timestamp X + 10, the user reopens Firefox for the first time. On startup, the fonts are moved by the C++ RFP fonts code. It sets the 'last font update' pref to X + 10
5. the blocklist client updates the font list from the server which lists a last modified time for the font of X + 5
6. we don't download that font because our local 'last update' time is X + 10

That doesn't seem right. It seems like the timestamp should be set when we finish downloading fonts. Am I misunderstanding part of the code?


Related super-nitty nit: the JS constant for the pref is "RFP_FONTS_LAST_UPDATE_TIME_PREF" and the C++ one is "RFP_FONT_LAST_UPDATE_TIME_PREF". They differ by the "S" in "FONT(S)". Please can we make them the same. :-)
(Same nit for the other constants that are the same across these files.)

> > Thank you for moving all of the JS IO to OS.File. Unfortunately it seems the C++ code is still using main-thread IO. Though I re-read the last comments on the bug, I haven't seen this addressed, either.
> > 
> > 
> > Can we move this off the main thread, like to a runnable, maybe? (I'm very much not a C++ IO expert, so please check with other folks that that suggestion actually makes things better here, but off-hand that would seem better than what this is currently doing.)
> 
> I think we need to do this in the main thread since we need to move the
> fonts into the place before the initialization of gfxPlatformFontList, see
> comment 82. And replacing fonts after they got loaded might cause unexpected
> behaviors. The performance problem we had talked before in comment 84 and
> comment 85.

Ah, right, sorry for missing this. OK, I defer to Jonathan on how this code should behave.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review211388

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:50
(Diff revision 7)
> +#define RFP_FONT_DIR "rfp-fonts"
> +#define RFP_FONT_STAGING_DIR "rfp-fonts-staging"
> +#define RFP_FONT_LAST_UPDATE_TIME_PREF "privacy.resistFingerprinting.fonts.last_update_time"
> +#define RFP_FONT_NEED_MOVE_PREF "privacy.resistFingerprinting.fonts.need_move"

Nitty nit: naming consistency, see bug 1336208 comment 131 (just making sure this is tracked in mozreview because I messed up and replied on bugzilla)
(In reply to Tim Huang[:timhuang] (OOO: 12/6 ~ 12/8) from comment #128)
> > Fun things: what happens if:
> > 
> > 1) user A runs Firefox and installs fonts in GreD
> > 2) user B runs Firefox and realizes fonts can (should?) be updated, but has
> > no write access to GreD
> 
> Unfortunately, there will be one problem in this case for the currect patch
> I wrote; the fonts in the profile directory of the user B won't be
> completed. Sorry, I didn't think straight before. Followings are what going
> to happen when user B downloads fonts:
>   1. The user B will check the up-to-dateness of fonts in GreD since the
> 'rfp-fonts' directory doesn't exist in its profile directory yet.
>   2. The user B will figure out some fonts need to be updated and download
> them into its staging directory.
>   3. At next startup of Firefox, only these new downloaded fonts are moved
> into 'rfp-fonts' directory in the profile directory. Those fonts that don't
> get updated won't be there. That's to say, the fonts are not completed for
> fingerprinting resistance.
> 
> One solution for this is that we can copy those fonts that are not updated
> into staging directory from GreD at step 2. So, the staging directory will
> contain all fonts we need.

I address this issue by moving fonts in GreD into profile directory in nsRFPService.cpp when we moving staging files. The reason why I changed to use nsRFPService.cpp to do this is that the code would be simpler in there. And it is hard to check a directory is writable or not asynchronously in JS code, it seems OS.File doesn't have this method. In addition, the overall file copy would be the same for both approaches.
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review212356


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/components/resistfingerprinting/nsRFPService.cpp:472
(Diff revision 8)
> +      NS_ENSURE_SUCCESS_VOID(rv);
> +    } else {
> +      NS_ENSURE_SUCCESS_VOID(rv);
> +    }
> +
> +    fontsMoved = true;

Warning: Value stored to 'fontsmoved' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
@Tim Huang & co

Just a quick question. Will this download the same fonts for every OS?

If so, that's OK from a fingerprinting perspective, but may be payload heavy (although I see Tor Browser Bundle do this). Bug 1404608 proposes, due to so many issues/breakages with OS spoofing in the UA (eg google doc issues, android not getting mobile pages, etc, as well as the OS being easy to determine) to limit the OS to four possibilities. If this was a go ahead, then you could potentially reduce the payload by having four sets of fonts - Windows set, linux set, android set, mac set.
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review212536

r=me with the below issues fixed. Note that I'm a little concerned about test coverage here, see below.

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:78
(Diff revisions 7 - 9)
>        // We set the pref here for telling nsRFPService to move fonts from staging
>        // directory to the font directory.
>        Services.prefs.setBoolPref(RFP_FONTS_NEED_MOVE_PREF, true);
>  
> -      // We notify the observer for telling fonts have been downloaded successfully.
> -      // This is for testing purpose.
> +      // Record the last time of the successful downloading.
> +      Services.prefs.setIntPref(RFP_FONTS_LAST_UPDATE_TIME_PREF, Date.now());

This doesn't work, you still need the `* 1000` hack, even when calling this API from JS:

Here's some sample console output:
```
> Services.prefs.setIntPref("sdgsdfgsdfgsdfgdsfg", Date.now())
< undefined
> Services.prefs.getIntPref("sdgsdfgsdfgsdfgdsfg")
< 936321606
> Date.now()
< 1512764820294
```

If tests didn't catch this, that's concerning. Is this part of the code covered by tests?

::: toolkit/components/resistfingerprinting/FontsDownloader.jsm:144
(Diff revisions 7 - 9)
>  
>        // Check the local font file and verify whether it is up-to-date.
>        try {
> -        let localFontData = await OS.File.read(localFontPath);
> +        let localFontStat = await OS.File.stat(localFontPath);
>  
> -        if (await verifyHash(localFontData, font.attachment.hash, "SHA-256")) {
> +        if (localFontStat.size === font.attachment.size) {

OK, so now we're assuming that any font changes will always involve a change in size? Is that OK? I don't know enough about fonts to be sure. My earlier suggestion was meant as an optimization - we could choose to test the hash iff the size of the file matches, but there's no point hashing if the filesize doesn't match.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:442
(Diff revisions 7 - 9)
> +      rv = destFontDir->Exists(&isExists);
> +      NS_ENSURE_SUCCESS_VOID(rv);
> +
> +      // If the font directory doesn't exist in the profile directory and this
> +      // profile has no right to access GreD, this means it's the first time
> +      // this profile try to move fonts from staging direcotry. In this case,

Nit: 'tries to move fonts from the staging directory'

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:443
(Diff revisions 7 - 9)
> +      NS_ENSURE_SUCCESS_VOID(rv);
> +
> +      // If the font directory doesn't exist in the profile directory and this
> +      // profile has no right to access GreD, this means it's the first time
> +      // this profile try to move fonts from staging direcotry. In this case,
> +      // we need to first copy all fonts into profile directory from the

Nit: *the* profile directory
Attachment #8901697 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review212536

> This doesn't work, you still need the `* 1000` hack, even when calling this API from JS:
> 
> Here's some sample console output:
> ```
> > Services.prefs.setIntPref("sdgsdfgsdfgsdfgdsfg", Date.now())
> < undefined
> > Services.prefs.getIntPref("sdgsdfgsdfgsdfgdsfg")
> < 936321606
> > Date.now()
> < 1512764820294
> ```
> 
> If tests didn't catch this, that's concerning. Is this part of the code covered by tests?

The current test will use a fake time on this, so it won't catch this problem. I will fix this and add a test around this.

> OK, so now we're assuming that any font changes will always involve a change in size? Is that OK? I don't know enough about fonts to be sure. My earlier suggestion was meant as an optimization - we could choose to test the hash iff the size of the file matches, but there's no point hashing if the filesize doesn't match.

Sorry, I misunderstood your comment. I will address this.
Whiteboard: [tor][fingerprinting][gfx-noted][fp:m3] → [tor][fingerprinting][gfx-noted][fp:m3][fp-triaged]
Flags: needinfo?(tom)
Assignee: artines1 → tom
Should we proceed with the kinto configuration?
(In reply to Rémy Hubscher (:natim) from comment #147)
> Should we proceed with the kinto configuration?

I'm going to pick this up from Tim and try to finish it up, so yes please proceed and I will try to get up to speed on what needs to be done here.
The kinto configuration has been deployed to stage, I invite you to try it to make sure you can add fonts, review them validate the signature and download them in your client code.

https://kinto-writer.stage.mozaws.net/v1/admin/ (You will need a VPN connection to access it.)
Comment on attachment 8901694 [details]
Bug 1336208 - Part 2: Making the gfxPlatformFontList to use fonts directory in profile folder if it exists for bundled fonts.

https://reviewboard.mozilla.org/r/173120/#review223210

I think this should be fine, except that I have one question regarding the Linux implementation, see below. That may or may not be a problem, I'm unsure. Clearing the r? flag for now; please re-request review after confirming how that scenario is handled.

::: gfx/thebes/gfxDWriteFontList.cpp:1910
(Diff revision 5)
> +
> +    RefPtr<IDWriteFontCollection> collection =
> +      CreateBundledFontsCollectionFromDir(aFactory, localDir);
> +
> +    return collection.forget();
> +

nit: lose the spare blank line

::: gfx/thebes/gfxFcPlatformFontList.cpp:2200
(Diff revision 5)
>  void
>  gfxFcPlatformFontList::ActivateBundledFonts()
>  {
>      if (!mBundledFontsInitialized) {
>          mBundledFontsInitialized = true;

Putting the whole of this method within the mBundledFontsInitialized flag check means all this will happen only on the first call to InitFontList(), and will be a no-op on any subsequent call. The old code used to remember the path, and re-do the FcConfigAppFontAddDir call on any later InitFontList().

I'm not sure at this point if that was actually necessary; what happens if the fontconfig configuration is changed at runtime (e.g. the user manually installs some new font packages in their distro), triggering a font-list rebuild? Do the bundled fonts continue to work after that, without repeating the FcConfigAppFontAddDir call here?

This may be fine, but I think it should be specifically tested -- have you tried such an experiment?
Attachment #8901694 - Flags: review?(jfkthame)
Comment on attachment 8901697 [details]
Bug 1336208 - Part 5: Implementing the font downloading for fingerprinting resistance.

https://reviewboard.mozilla.org/r/173126/#review223232

LGTM. I didn't try to review the JS code, assuming Gijs has already done that -- and actually speaks the language. :)

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:382
(Diff revision 10)
> +nsRFPService::MaybeMoveFontFiles()
> +{
> +  if (!XRE_IsParentProcess()) {
> +    return;
> +  }
> +
> +  // First, we check the pref for whether we need to move staging fonts.
> +  if (!Preferences::GetBool(RFP_FONTS_NEED_MOVE_PREF)) {
> +    return;
> +  }

Given that the platform code does not support dynamic changes to the RFP fonts collection, I think we should have an assertion here to confirm that we're running early enough in startup. (AFAICS, that should be the case, but in case someone rearranges things some day...)

I think you could test

    MOZ_ASSERT(!gfxPlatform::Initialized(), "must run before gfxPlatform initialization!");

here to make sure we're safe.

Perhaps should also assert that we're on the main thread, because if we ever tried doing this from a different thread it would then be racing with platform initialization, which would be bad...

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:474
(Diff revision 10)
> +  // After moving all fonts, we remove the staging directory.
> +  rv = stagingDir->Remove(true);

The staging dir should be empty by now, right? So could we assert that?

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:530
(Diff revision 10)
> +    }
> +  }
> +
> +  if (!strcmp(PROFILE_DO_CHANGE_TOPIC, aTopic)) {
> +    // We move staging font files into the font directory as soon as the profile
> +    // is available. This stage is before the initialization of gfxPlatfromFontList.

typo, s/gfxPlatfromFontList/gfxPlatformFontList/
Attachment #8901697 - Flags: review?(jfkthame) → review+
(In reply to Rémy Hubscher (:natim) from comment #149)
> The kinto configuration has been deployed to stage, I invite you to try it
> to make sure you can add fonts, review them validate the signature and
> download them in your client code.
> 
> https://kinto-writer.stage.mozaws.net/v1/admin/ (You will need a VPN
> connection to access it.)

Do I need to be in a special VPN group?  I can get an IP address via DNs, but cannot connect:

> nc: connectx to kinto-writer.stage.mozaws.net port 443 (tcp) failed: Connection refused
Flags: needinfo?(tom) → needinfo?(rhubscher)
Depends on: 1435672
Yes you need to. I created Bug 1435672 to verify that you are.
Flags: needinfo?(rhubscher)
Assignee: tom → nobody
Priority: P1 → P3
Attachment #8901698 - Flags: review?(jfkthame)
Attachment #8901699 - Flags: review?(jfkthame)
Whiteboard: [tor][fingerprinting][gfx-noted][fp:m3][fp-triaged] → [tor][fingerprinting][gfx-noted][fp:m3]
No longer blocks: uplift_tor_fingerprinting
Whiteboard: [tor][fingerprinting][gfx-noted][fp:m3] → [fingerprinting][gfx-noted][fp-triaged]

As someone whose font list is literally unique, is there a workaround for the time being?

(In reply to michael from comment #154)

As someone whose font list is literally unique, is there a workaround for the time being?

To limit the fonts returned somewhat, you could use the whitelists used by Tor Browser per platform. Note that, for example, in Windows, Tor Browser ships some Noto Sans fonts, which Windows wouldn't normally have. This list contains non-Western characters, so you can't put it in a user.js for example.

e.g, the Windows TB whitelist is

Arial, Batang, 바탕, Cambria Math, Courier New, Euphemia, Gautami, Georgia, Gulim, 굴림, GulimChe, 굴림체, Iskoola Pota, Kalinga, Kartika, Latha, Lucida Console, MS Gothic, ＭＳ ゴシック, MS Mincho, ＭＳ 明朝, MS PGothic, ＭＳ Ｐゴシック, MS PMincho, ＭＳ Ｐ明朝, MV Boli, Malgun Gothic, Mangal, Meiryo, Meiryo UI, Microsoft Himalaya, Microsoft JhengHei, Microsoft JengHei UI, Microsoft YaHei, 微软雅黑, Microsoft YaHei UI, MingLiU, 細明體, Noto Sans Buginese, Noto Sans Khmer, Noto Sans Lao, Noto Sans Myanmar, Noto Sans Yi, Nyala, PMingLiU, 新細明體, Plantagenet Cherokee, Raavi, Segoe UI, Shruti, SimSun, 宋体, Sylfaen, Tahoma, Times New Roman, Tunga, Verdana, Vrinda, Yu Gothic UI

This would reduce the number of fonts listed (i.e think about all the additional fonts you get when you install MS Office, Photoshop, etc: these wouldn't show up), but wouldn't reduce your entropy, until we all did it. And I'm not sure what affect on usability not having those Noto Sans fonts would do.

Type: defect → enhancement
Keywords: feature
See Also: → 1582687

Where can i see which fonts are active with privacy.resistFingerprinting = true ?
Is there a list of all the fonts allowed to use with privacy.resistFingerprinting = true ?

Thanks a lot.
This makes whitelisting some fonts much easier, when i know which fonts are used with privacy.resistFingerprinting = true.

IMHO all the fonts should be the same for all the platforms.

Ubuntu 18.04
The fc-list fontlist is too long to display.

I found the fonts that are necessary for the firefox UI to display correct.
When i put these 2 fonts on my whitelist the hieroglyphs are gone.
FreeSerif
Garuda

But the firefox UI should be independent from the font whitelist.
The firefox UI should always be allowed to use all system fonts.
If the used fonts for the firefox UI will change, i am pretty sure, i will have hieroglyphs again without changing the behaviour of the firefox UI.

Flags: needinfo?(arthur)
Severity: normal → S3
See Also: → 1850672

Has something changed in Firefox 123?

I might have a new issue, see bug 1880901

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: