Maintenance: Regular expression to get code location in logger may take more time to make some inputs
Closed this issue · 8 comments
Summary
The regular expression used in getCodeLocation may run slow on strings starting with '(' and with many repetitions of '(('. So, we need to prevent this behaviour by modifying the regular expression
Why is this needed?
Some regular expressions take a long time to match certain input strings to the point where the time it takes to match a string of length n is proportional to nk or even 2n. Such regular expressions can negatively affect performance, or even allow a malicious user to perform a Denial of Service ("DoS") attack by crafting an expensive input string for the regular expression to match.
Which area does this relate to?
Logger
Solution
Updating the regular expression (adding an additional ( to the first capture group) to the following might prevent such catastrophic backtracking:
From:
const regex = /\(([^)]*?):(\d+?):(\d+?)\)\\?$/;To:
const regex = /\(([^()]*?):(\d+?):(\d+?)\)\\?$/;Acknowledgment
- This request meets Powertools for AWS Lambda (TypeScript) Tenets
- Should this be considered in other Powertools for AWS Lambda languages? i.e. Python, Java, and .NET
Future readers
Please react with 👍 and your use case to help us understand customer demand.
@leandrodamascena need your opinion here: can these characters that the finding be even present in Lambda's managed runtime file systems?
The regex described in the issue is the one used by Logger exclusively to interpret the stack trace of an error and find the location of the code.
I'm trying to understand if the attack vector described above is even realistic and we should invest time in finding an alternative or not.
@leandrodamascena need your opinion here: can these characters that the finding be even present in Lambda's managed runtime file systems?
The Lambda environment is highly isolated by default, as it runs on Firecracker. Lambda images are rootless by default, which means even you could have something malicious: 1/ this will be isolated to your sandbox, 2/ any memory exhaustion will be killed by the Lambda control plane if you exceed the memory limit set for your function, and 3/ it cannot affect files on the filesystem, as it cannot write to any location other than /tmp.
More info here: https://docs.aws.amazon.com/lambda/latest/dg/images-create.html
The regex described in the issue is the one used by Logger exclusively to interpret the stack trace of an error and find the location of the code.
I'm trying to understand if the attack vector described above is even realistic and we should invest time in finding an alternative or not.
I can't say much about performance-wise because I haven't run any stress tests with this, but it seems like this can happen in very specific scenarios where you have a lot of ( in the string. From the possible ReDoS attack vector, I think it's an exaggeration that this could damage your system. Let me put it better: every complex regexp that accepts arbitrary values can be a potential attack vector if the string is not sanitized, it's because it can consume a lot of CPU and memory to process the regexp with the a given value. But for this to happen, I believe it has to be a huge string, something like const xxxxxxx = "(((((xw((x".repeat(10000000) + "blalbalblabla)"; and then it can cause exhaustion. But even so, the stacktrace would have to be huge and it still has to be intentional. I don't see how an external attack could cause this.
Furthermore, if this happens, it will be outside of the Lambda environment, and we don't support it.
That said, I would invest at most half a day trying to figure out a better regular expression for this, and if I can't, I'd happily ignore this warning.
It's probably unrealistic to have such kind of string in the stack trace but I checked the number of steps with the regex I had suggested in the solution of this issue and got these results.
The original regex which had the problem of backtracking
The updated one, which ignores the (, specifically for this kind of string
The updated one, which ignores the
(, specifically for this kind of string
If you are sure that updating this won't break anything, then why not?
Any decision on this issue?
I'll create a PR today for this with the suggested change. It passes the existing tests and I haven't found any edge cases which would fail, yet.
Warning
This issue is now closed. Please be mindful that future comments are hard for our team to see.
If you need more assistance, please either reopen the issue, or open a new issue referencing this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.
This is now released under v2.26.0 version!