Skip to content

Commit

Permalink
Generate file handle IDs randomly
Browse files Browse the repository at this point in the history 
Mitigates a hypothetical attack where a malicious extension could enumerate
all previously opened file IDs and then attempt to overwrite them as they
were just sequential numbers.
  • Loading branch information
@GarboMuffin
GarboMuffin committed Dec 19, 2024
1 parent 87bba95 commit 92142c7
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 46 deletions.
124 changes: 82 additions & 42 deletions src-main/windows/editor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,36 +187,75 @@ const isChildPath = (parent, child) => {
return !!relative && !relative.startsWith('..') && !path.isAbsolute(relative);
};

/** @type {Set<string>} */
const allFileIDs = new Set();

/**
* @returns {string} A unique string.
*/
const generateFileId = () => {
let result;
let tries = 0;

do {
tries++;
if (tries > 50) {
// Should never happen...
throw new Error('Failed to generate file ID');
}

result = 'desktop_file_id{';

// >200 bits of randomness; impractical to brute force.
// Math.random() is not cryptographically secure, but even if someone can reverse it, they would
// still only be able to access files that were already opened, so impact is not that big.
const soup = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz';
for (let i = 0; i < 40; i++) {
result += soup[Math.floor(Math.random() * soup.length)];
}

result += '}';
} while (allFileIDs.has(result));

allFileIDs.add(result);
return result;
};

class EditorWindow extends ProjectRunningWindow {
/**
* @param {OpenedFile|null} file
* @param {OpenedFile|null} initialFile
* @param {boolean} isInitiallyFullscreen
*/
constructor (file, isInitiallyFullscreen) {
constructor (initialFile, isInitiallyFullscreen) {
super();

// This file ID system is not quite perfect. Ideally we would completely revoke permission to access
// old projects after you load the next one, but our handling of file handles in scratch-gui is
// pretty bad right now, so this is the best compromise.
this.openedFiles = [];
this.activeFileIndex = -1;
/**
* Ideally we would revoke access after loading a new project, but our file handle handling in
* the GUI isn't robust enough for that yet. We do at least use random file handle IDs which
* makes it much harder for malicious code in the renderer process to enumerate all previously
* opened IDs and overwrite them.
* @type {Map<string, OpenedFile>}
*/
this.openedFiles = new Map();
this.activeFileId = null;

if (file !== null) {
this.openedFiles.push(file);
this.activeFileIndex = 0;
if (initialFile !== null) {
this.activeFileId = generateFileId();
this.openedFiles.set(this.activeFileId, initialFile);
}

this.openedProjectAt = Date.now();

const getFileByIndex = (index) => {
if (typeof index !== 'number') {
throw new Error('File ID not number');
}
const value = this.openedFiles[index];
if (!(value instanceof OpenedFile)) {
/**
* @param {string} id
* @returns {OpenedFile}
* @throws if invalid ID
*/
const getFileById = (id) => {
if (!this.openedFiles.has(id)) {
throw new Error('Invalid file ID');
}
return this.openedFiles[index];
return this.openedFiles.get(id);
};

this.window.webContents.on('will-prevent-unload', (event) => {
Expand Down Expand Up @@ -263,14 +302,11 @@ class EditorWindow extends ProjectRunningWindow {
});

ipc.handle('get-initial-file', () => {
if (this.activeFileIndex === -1) {
return null;
}
return this.activeFileIndex;
return this.activeFileId;
});

ipc.handle('get-file', async (event, index) => {
const file = getFileByIndex(index);
ipc.handle('get-file', async (event, id) => {
const file = getFileById(id);
const {name, data} = await file.read();
return {
name,
Expand Down Expand Up @@ -301,18 +337,18 @@ class EditorWindow extends ProjectRunningWindow {
this.window.setDocumentEdited(changed);
});

ipc.handle('opened-file', (event, index) => {
const file = getFileByIndex(index);
ipc.handle('opened-file', (event, id) => {
const file = getFileById(id);
if (file.type !== TYPE_FILE) {
throw new Error('Not a file');
}
this.activeFileIndex = index;
this.activeFileId = id;
this.openedProjectAt = Date.now();
this.window.setRepresentedFilename(file.path);
});

ipc.handle('closed-file', () => {
this.activeFileIndex = -1;
this.activeFileId = null;
this.window.setRepresentedFilename('');
});

Expand All @@ -331,14 +367,16 @@ class EditorWindow extends ProjectRunningWindow {
return null;
}

const file = result.filePaths[0];
settings.lastDirectory = path.dirname(file);
const filePath = result.filePaths[0];
settings.lastDirectory = path.dirname(filePath);
await settings.save();

this.openedFiles.push(new OpenedFile(TYPE_FILE, file));
const id = generateFileId();
this.openedFiles.set(id, new OpenedFile(TYPE_FILE, filePath));

return {
id: this.openedFiles.length - 1,
name: path.basename(file)
id,
name: path.basename(filePath)
};
});

Expand All @@ -356,30 +394,32 @@ class EditorWindow extends ProjectRunningWindow {
return null;
}

const file = result.filePath;
const filePath = result.filePath;

const unsafePath = getUnsafePaths().find(i => isChildPath(i.path, file));
const unsafePath = getUnsafePaths().find(i => isChildPath(i.path, filePath));
if (unsafePath) {
// No need to wait for the message box to close
// No need to block until the message box is closed
dialog.showMessageBox(this.window, {
type: 'error',
title: APP_NAME,
message: translate('unsafe-path.title'),
detail: translate(`unsafe-path.details`)
.replace('{APP_NAME}', unsafePath.app)
.replace('{file}', file),
.replace('{file}', filePath),
noLink: true
});
return null;
}

settings.lastDirectory = path.dirname(file);
settings.lastDirectory = path.dirname(filePath);
await settings.save();

this.openedFiles.push(new OpenedFile(TYPE_FILE, file));
const id = generateFileId();
this.openedFiles.set(id, new OpenedFile(TYPE_FILE, filePath));

return {
id: this.openedFiles.length - 1,
name: path.basename(file)
id,
name: path.basename(filePath)
};
});

Expand All @@ -390,8 +430,8 @@ class EditorWindow extends ProjectRunningWindow {
};
});

ipc.on('start-write-stream', async (startEvent, index) => {
const file = getFileByIndex(index);
ipc.on('start-write-stream', async (startEvent, id) => {
const file = getFileById(id);
if (file.type !== TYPE_FILE) {
throw new Error('Not a file');
}
Expand Down
2 changes: 1 addition & 1 deletion src-preload/editor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ ipcRenderer.on('export-project-to-port', (e) => {
window.addEventListener('message', (e) => {
if (e.source === window) {
const data = e.data;
if (data && typeof data.ipcStartWriteStream === 'number') {
if (data && typeof data.ipcStartWriteStream === 'string') {
ipcRenderer.postMessage('start-write-stream', data.ipcStartWriteStream, e.ports);
}
}
Expand Down
6 changes: 3 additions & 3 deletions src-renderer-webpack/editor/gui/filesystem-api.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ const toUnit8Array = (contents) => {

class WrappedFileWritable {
/**
* @param {number} id File ID from main
* @param {string} id File ID from main
*/
constructor (id) {
this._channel = new MessageChannel();

/** @type {Map<number, {resolve: () => void, reject: (error: unknown) => void}>} */
/** @type {Map<string, {resolve: () => void, reject: (error: unknown) => void}>} */
this._callbacks = new Map();
this._lastMessageId = 1;

Expand Down Expand Up @@ -106,7 +106,7 @@ class WrappedFileWritable {

class WrappedFileHandle {
/**
* @param {number} id File ID from main.
* @param {string} id File ID from main.
* @param {string} name Name including file extension.
*/
constructor (id, name) {
Expand Down

0 comments on commit 92142c7

Please sign in to comment.