webpro / reveal-md

reveal.js on steroids! Get beautiful reveal.js presentations from any Markdown file
MIT License
3.74k stars 416 forks source link

Add patch for arbitrary file read #424

Closed W0rty closed 2 years ago

W0rty commented 2 years ago

This patch will do two things :

W0rty commented 2 years ago

As I've fixed two vulnerabilities inside your application and bypass your restriction, will it be possible to have a CVE for the arbitrary file read ?

Thanks

webpro commented 2 years ago

Thanks @W0rty!

Happy to merge this. Could you please apply Prettier formatting and remove console.log debug statements?

As I've fixed two vulnerabilities inside your application and bypass your restriction, will it be possible to have a CVE for the arbitrary file read ?

What is a CVE? Do you expect something from me here?

W0rty commented 2 years ago

Yes sorry,

A CVE is a reference that allows to reference the different vulnerabilities found in an application, in this case reveal-md. See: https://www.cve.org/

I can make the request myself, but it is better if the patch is applied first.

mtoohey31 commented 1 year ago

Hi @W0rty, sorry that I'm bugging you about this a few months after the change. I'm the person who last modified the related code for reading files though (in c140f33f1e402cd53c39b274963da0332d3a83cc), so I was wondering if you wouldn't mind demonstrating the exploit for arbitrary file read, so I can figure out where I messed up.

My understanding is that the path.join call normalizes the result (according to the docs), meaning it shouldn't contain any ".." segments. My thought processes was that because of that, the startsWith check would be sufficient to filter out any paths to files outside of the directory, but maybe there are cases I've missed where that's not the case?

Again, sorry to bug you, I'd just like to understand where I went wrong, so I can be confident when dealing with this type of thing in the future. Thanks!

W0rty commented 1 year ago

Hello,

In the docs you can read the following statement :

path.join('/foo', 'bar', 'baz/asdf', 'quux', '..');
// Returns: '/foo/bar/baz/asdf'

As you said, you thought that the output will be /foo/bar/baz/asdf/quux/, but here the ".." is handled by the path.join method, which results in /foo/bar/baz/asdf.

If we take an exemple from your patch :

const dir = await getInitialDir();
  const filePath = path.join(dir, decodeURIComponent(req.url).replace(/\?.*/, ''));
  const markup = await renderFile(filePath);
  res.send(markup);
  if (filePath.startsWith(dir)) {
    const markup = await renderFile(filePath);
    res.send(markup);
    return;
  }

Here we know that dir variable, so we can give as input ${dir}/../../../../etc/passwd, which, with path.join, results in /etc/passwd :)

mtoohey31 commented 1 year ago

Thanks for the quick response!

It looks like you may have mis-copied a few lines or missed the patch markers though. For the sake of clarity, the patch itself was:

diff --git a/lib/render.js b/lib/render.js
index 6ed3764..692a59d 100644
--- a/lib/render.js
+++ b/lib/render.js
@@ -64,8 +64,12 @@ const renderFile = async (filePath, extraOptions) => {
 module.exports = async (req, res) => {
   const dir = await getInitialDir();
   const filePath = path.join(dir, decodeURIComponent(req.url).replace(/\?.*/, ''));
-  const markup = await renderFile(filePath);
-  res.send(markup);
+  if (filePath.startsWith(dir)) {
+    const markup = await renderFile(filePath);
+    res.send(markup);
+    return;
+  }
+  res.status(401).send("Forbidden: Only slides inside the initial directory can be accessed.");
 };

 module.exports.slidify = slidify;

...meaning that the code after my changes would be:

  const dir = await getInitialDir();
  const filePath = path.join(dir, decodeURIComponent(req.url).replace(/\?.*/, ''));
  if (filePath.startsWith(dir)) {
    const markup = await renderFile(filePath);
    res.send(markup);
    return;
  }
  res.status(401).send("Forbidden: Only slides inside the initial directory can be accessed.");

If the result of path.join is /etc/passwd, won't filePath.startsWith(dir) be false, leading to the 401?