Skip to content

Commit

Permalink
[mirotalksfu] - fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@miroslavpejic85
miroslavpejic85 committed Aug 5, 2024
1 parent 2e18650 commit 760d01c
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 16 deletions.
10 changes: 1 addition & 9 deletions app/src/Server.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ function startServer() {

// Rec_test_2024_08_03_16_17_01.webm

if (!isValidRecFileNameFormat(fileName)) {
if (!fileName.startsWith('Rec_') && !fileName.endsWith('.webm')) {

This comment has been minimized.

Sign in to view

Copy link
@realArcherL

realArcherL Aug 5, 2024

I think this would still leave room for errors. I think we should filter out the special symbols as well. Allow, only "_" and remove everything else.

I worked on a quick fix for CVE-2024-39918 . Maybe using something like a function which the author used in their file here: https://github.com/jasonraimondi/url-to-png/blob/main/src/lib/utils.ts#L17C1-L26C2

export function slugify(text: string) {
  return text
    .toString()
    .toLowerCase()
    .replace(/\s+/g, "-") // Replace spaces with -
    .replace(/[^\w\-]+/g, "") // Remove all non-word chars
    .replace(/\-\-+/g, "-") // Replace multiple - with single -
    .replace(/^-+/, "") // Trim - from start of text
    .replace(/-+$/, ""); // Trim - from end of text
}

Or methods suggested in this article: https://www.stackhawk.com/blog/node-js-path-traversal-guide-examples-and-prevention/

This comment has been minimized.

Sign in to view

Copy link
@realArcherL

realArcherL Aug 5, 2024 •
edited
Loading

Moreover, this only takes care of the Path Traversal issue. We also need to ensure we remove the unrestricted file upload. Anyone can upload anything. More on it here: OWASP GUIDE

I think a more comprehensive solution would be to check for the MIME type and reject upload OR/AND rename the file extension ourselves so that even if the file is uploaded it is restricted to the format we want.

Some good examples of this are here

  1. Checking the MIME type of an SVG(Although not sure how well will it work for videos)

  2. You can follow this guide OWASP GUIDE to build more defense in-depth features for file uploads.

This comment has been minimized.

Sign in to view

Copy link
@realArcherL

realArcherL Aug 5, 2024

I can push a PR by weekend if it helps 🙏
log.warn('[RecSync] - Invalid file name', fileName);
return res.status(400).send('Invalid file name');
}
Expand Down Expand Up @@ -2960,12 +2960,4 @@ function startServer() {
}
}
}

// Utils...

function isValidRecFileNameFormat(input) {
const pattern =
/^Rec_(?:[A-Za-z0-9-_]+|[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12})_\d{4}_\d{2}_\d{2}_\d{2}_\d{2}_\d{2}\.(webm)$/;
return pattern.test(input);
}
}
8 changes: 1 addition & 7 deletions cloud/server.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ app.post('/recSync', (req, res) => {
return res.status(400).send('Filename not provided');
}

if (!isValidRecFileNameFormat(fileName)) {
if (!fileName.startsWith('Rec_') && !fileName.endsWith('.webm')) {
log.warn('[RecSync] - Invalid file name', fileName);
return res.status(400).send('Invalid file name');
}
Expand Down Expand Up @@ -86,9 +86,3 @@ app.post('/recSync', (req, res) => {
app.listen(port, () => {
log.debug(`Server is running on http://localhost:${port}`);
});

function isValidRecFileNameFormat(input) {
const pattern =
/^Rec_(?:[A-Za-z0-9-_]+|[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12})_\d{4}_\d{2}_\d{2}_\d{2}_\d{2}_\d{2}\.(webm)$/;
return pattern.test(input);
}

0 comments on commit 760d01c

Please sign in to comment.