1488737 - Race Condition in downloadAddonToTemporaryFile()

Status: RESOLVED FIXED

(Firefox :: Normandy Client, defect)

Description

[u621419]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180808222917

Steps to reproduce:

In toolkit/components/normandy/lib/AddonStudies.jsm the function downloadAddonToTemporaryFile() downloads content into a temporary file.

  async downloadAddonToTemporaryFile(addonUrl) {
    const response = await fetch(addonUrl);
    if (!response.ok) {
      throw new Error(`Download for ${addonUrl} failed: ${response.status} ${response.statusText}`);
    }

    // Create temporary file to store add-on.
    const path = OS.Path.join(OS.Constants.Path.tmpDir, "study.xpi");
    const {file, path: uniquePath} = await OS.File.openUnique(path);

    // Write the add-on to the file
    try {
      const xpiArrayBufferView = new Uint8Array(await response.arrayBuffer());
      await file.write(xpiArrayBufferView);
    } finally {
      await file.close();
    }

    return new FileUtils.File(uniquePath);
  },


This creates a temporary file in the system temp directory and uses uniquePath() to generate a temporary filename for this.

The optional options parameter is missing in the call to uniquePath().

AbstractFile.openUnique = function openUnique(path, options = {}) {
  let mode = {
    create: true
  };

  let dirName = Path.dirname(path);
  let leafName = Path.basename(path);
  let lastDotCharacter = leafName.lastIndexOf(".");
  let fileName = leafName.substring(0, lastDotCharacter != -1 ? lastDotCharacter : leafName.length);
  let suffix = (lastDotCharacter != -1 ? leafName.substring(lastDotCharacter) : "");
  let uniquePath = "";
  let maxAttempts = options.maxAttempts || 99;
  let humanReadable = !!options.humanReadable;
  const HEX_RADIX = 16;
  // We produce HEX numbers between 0 and 2^24 - 1.
  const MAX_HEX_NUMBER = 16777215;

  try {
    return {
      path,
      file: OS.File.open(path, mode)
    };
  } catch (ex) {
    if (ex instanceof OS.File.Error && ex.becauseExists) {
      for (let i = 0; i < maxAttempts; ++i) {
        try {
          if (humanReadable) {
            uniquePath = Path.join(dirName, fileName + "-" + (i + 1) + suffix);
          } else {
            let hexNumber = Math.floor(Math.random() * MAX_HEX_NUMBER).toString(HEX_RADIX);
            uniquePath = Path.join(dirName, fileName + "-" + hexNumber + suffix);
          }
          return {
            path: uniquePath,
            file: OS.File.open(uniquePath, mode)
          };

If the optional.humanReadable variable is not set, the code tries filename-1, filename-2 and so on, which is perfectly guessable. In certain cases, this could be abused via race conditions. 

On older NFS (see https://lwn.net/Articles/251004/) shares, the O_EXCL parameter, which was added in the open() wrapper did not work as expected.

Other calls to openUnique() should be audited as well. The calls to openUnique() in file mozapps/extensions/-internal/-ProductAddonChecker.jsm, function downloadFile() and file mozapps/extensions/internal/XPIInstall.jsm, function downloadAddon() seem vulnerable as well.

X41 advises to generate random numbers to append in all of the cases. Furthermore, a proper random number generator should be used.

Comment 1

[Michael Cooper [:mythmon]]

In bug 1440780, the handling of add-on installation was changed, and downloadAddonToTemporaryFile was removed. I believe this is no longer a problem.

https://hg.mozilla.org/mozilla-central/rev/0068ee254d4a

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Luis, can you confirm that this issue is resolved now?

Comment 3

[u621419]

As far as I can tell, downloadAddonToTemporaryFile() has been removed in https://hg.mozilla.org/mozilla-central/rev/0068ee254d4a, so this should be resolved.