Hello! On Wed, Jul 12, 2023 at 05:45:34PM -0400, Jonathan Leitschuh wrote:
> Hi Maxim, > > Pleasure to meet you! > > I've also included Munawar, the CEO of Open Refactory in this email chain. > > > (It looks like you aren't subscribed to nginx-devel@, so the message did > not reach the mailing list: it only accepts messages from subscribers. > I've preserved it in Cc: though, so we can continue discussion there, with > more developers involved.) > > Do I need to subscribe to continue to get this thread included in that > email group? The website didn't explicitly say that I needed to subscribe > to send emails there, that might be a good addition. The http://nginx.org/en/support.html page says: "To post to a mailing list, an e-mail address that will be used for posting must first be subscribed." This applies to all mailing lists on the page, including nginx-devel@. Sorry if it wasn't clear. > > Indeed, the misconfiguration in question is a well-known one. Gixy, a > tool to detect various nginx misconfigurations, tries to detect it at least > since 2017: > > > > > https://github.com/yandex/gixy/blob/master/docs/en/plugins/aliastraversal.md > > From the documentation, they provide the following example request: > `/i../app/config.py`. I can't imagine a case where a request like > `/i../app/config.py` could ever be made legitimately by a user. Why is > `../app/config.py` appended to the alias `/data/w3/images/` in this case > without nginx throwing an exception as this is a blatant path traversal > attack? The "i.." string is a valid path component and there are no reasons why it shouldn't be allowed - there might be such files and/or directories on the disk. Similarly, location matching and alias as implemented in nginx (and documented) are expected to result in "../app/config.py" appended to "/data/w3/images/" for such a configuration. The question here is why the location prefix without trailing "/" was used in the configuration, and if it was intentional. > > Unfortunately, I don't think there is an easy fix. Changing the way how > nginx locations work would be a major change, and will break a lot of valid > configurations where prefix matching is intentionally used. > > This sounds like a fix that could this go through a depreciation cycle, > where, nginx adds an intentional, opt-in, prefix-matching flag that, in > it's early incarnations, offers warnings to users for > deprecated configuration, and later, will fail-closed unless the flag is > present? Sure, major changes can be implemented in a way which warns users in advance. This still be a major change though, and the transition will require several years and will disturb a lot of users, including ones who aren't affected by the configuration issue in question. Further, it is not clear if it will help: nothing will stop users from using the explicit flag for prefix matching in an unsafe way without realizing it's unsafe. Given that, an explicit warning/note in the documentation looks like the way to go, at least for now. > > > If you think there is an easy fix, feel free to provide suggestions. > > On our side, between Munwar and myself, we have some significant experience > with bulk pull request generation to fix security vulnerabilities at-scale > across open source software. (See my Black Hat & DEF CON Talk on the topic > from last year > <https://github.com/jlleitschuh/#scaling-the-security-researcher-to-eliminate-oss-vulnerabilities-once-and-for-all> > ) > > Munawar and his team at OpenRefactory did an analysis of the 2k+ > repositories that GitHub flags with the above linked search. From his > team's analysis: > > - Majority of the bugs are in documentation. These can be easily filtered > out with the file type. We can consider bulk pull request generation adding > a bit that we are fixing a documentation to make it correct so that people > are not misinformed and left with a vulnerability. > - There are a few bugs in the actual config files. In such cases, you may > split the projects based on the GH stars. The more popular ones should get > a stronger message communicating the risk to their end-users. > > We can likely massively clean-up this vulnerability across the OSS > ecosystem, but the long-term solution to this vulnerability, I believe, > will lie in the hands of the Nginx team. > > How can we work together here towards a world where this vulnerability is > something end-users opt-in to, with intention (usually via some flag), > rather than the world we are currently in, where this vulnerability appears > as a mistake? > > Cheers, > Jonathan Leitschuh > > > On Fri, Jul 7, 2023 at 7:21 PM Maxim Dounin <mdou...@mdounin.ru> wrote: > > > Hello! > > > > On Thu, Jul 06, 2023 at 12:05:25PM -0400, Jonathan Leitschuh wrote: > > > > > Hi Nginx Team, > > > > > > My name is Jonathan Leitschuh. I'm a Senior Software Security Researcher > > > for the Open Source Security Foundation: Project Alpha-Omega > > > <https://openssf.org/community/alpha-omega/>. “Alpha” works with the > > > maintainers of the most critical open source projects to help them > > identify > > > and fix security vulnerabilities, and improve their security posture. > > > “Omega” identified at least 10,000 widely deployed OSS projects where it > > > can apply automated security analysis, scoring, and remediation guidance > > to > > > their open source maintainer communities. > > > > > > Earlier today, on Twitter, I saw this discussion regarding > > > the long-standing the NGinx Alias traversal vulnerability: > > > https://twitter.com/infosec_au/status/1676072447156834304?s=20 > > > > > > In particular, this discussion centered on this, this recent blog post > > > published on the 3rd of this month. > > > https://labs.hakaioffsec.com/nginx-alias-traversal/ > > > > > > In that blog post, the researchers detail how, due to misconfiguration > > and > > > failing to specify the `/` character in the correct place can lead to > > path > > > traversal vulnerabilities. > > > > > > > > > > > > This vulnerability was previously discussed by Detectify in a blog post > > in > > > 2020: > > > https://blog.detectify.com/2020/11/10/common-nginx-misconfigurations/ > > > > > > This research originated with Orange Tsai back in 2018: > > > > > https://i.blackhat.com/us-18/Wed-August-8/us-18-Orange-Tsai-Breaking-Parser-Logic-Take-Your-Path-Normalization-Off-And-Pop-0days-Out-2.pdf > > > > > > The recent hakaioffsec blog also identifies this case where the > > > vulnerability also appears: > > > > > > > > > My question to both the Nginx Security team, and the Nginx Developers, > > why > > > not fix this vulnerability in Nginx itself such that it's safe by > > default, > > > but users can opt-in to the vulnerable behaviour if they so desire > > through > > > an explicit flag? Surely it would be fairly straightforward to identify > > > these vulnerable configurations and fix them for the user, or fail closed > > > (safe)? > > > > > > If you need evidence of the impact this vulnerability can have, the > > > above hakaioffsec blog post details how this vulnerability resulted in > > > a US$6000 bounty from BitWarden. > > > > > > This GitHub search, proposed by the above blog post, returns 2.2k results > > > on GitHub: > > > > > > > > https://github.com/search?q=%2Flocation+%5C%2F%5B_.a-zA-Z0-9-%5C%2F%5D*%5B%5E%5C%2F%5D%5B%5Cs%5D%5C%7B%5B%5Cs%5Cn%5D*alias+%5C%2F%5B_.a-zA-Z0-9-%5C%2F%5D*%5C%2F%3B%2F+&type=code > > > > > > I'm curious what is currently preventing this from being fixed upstream > > to > > > prevent end-users from accidentally implementing vulnerable > > configurations > > > like this. > > > > Thank you for your message. > > > > Indeed, the misconfiguration in question is a well-known one. > > Gixy, a tool to detect various nginx misconfigurations, tries to > > detect it at least since 2017: > > > > > > https://github.com/yandex/gixy/blob/master/docs/en/plugins/aliastraversal.md > > > > The root cause as I see it is that nginx locations (and alias) > > work with prefix strings, while unaware people often expect them > > to work with path components (directories) instead. Note the > > "Achieving Impact Without a Slash on Alias" section in the article > > you've referenced: a configuration like > > > > location /img { > > alias /var/images; > > } > > > > makes it possible to access "/var/images_confidential" via a > > request to "/img_confidential". This is because locations works > > on prefix strings, and due to lack of trailing "/" in the location > > prefix it does match "/img_confidential" request, and maps it to > > the file system according to the configuration. > > > > Similarly, the same misconfiguration can happen even without the > > "alias" directive at all. Consider the configuration: > > > > location / { deny all; } > > location /images { allow all; } > > > > In contrast to some might (incorrectly) assume, a request to > > "/images_confidential" will be allowed, and will return content as > > per the configuration. > > > > Unfortunately, I don't think there is an easy fix. Changing the > > way how nginx locations work would be a major change, and will > > break a lot of valid configurations where prefix matching is > > intentionally used. > > > > Meanwhile, it surely deserves some additional warnings/notes in > > the documentation, to emphasize the actual behaviour and potential > > pitfalls. Hopefully it would be enough to at least reduce the > > amount of such misconfigurations. > > > > If you think there is an easy fix, feel free to provide > > suggestions. > > > > (It looks like you aren't subscribed to nginx-devel@, so the > > message did not reach the mailing list: it only accepts messages > > from subscribers. I've preserved it in Cc: though, so we can > > continue discussion there, with more developers involved.) > > > > -- > > Maxim Dounin > > http://mdounin.ru/ > > -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
