Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CVE-2023-42282 #138

Closed
wants to merge 7 commits into from
Closed

Conversation

Cofgren
Copy link

@Cofgren Cofgren commented Feb 12, 2024

Hi,

I've made a patch for CVE-2023-42282 (handling of 0x7f.1 address in isPrivate).

Details of the vulnerability

Please review and merge if ok.

Thanks.

@indutny
Copy link
Owner

indutny commented Feb 12, 2024

Thanks for the fix! Could you elaborate on what RFC marks 0x7f.0.0.1 as a valid IPv4 address, and in what way this value can reach the Node.js application that uses this module?

If we go this way - we probably want to add support for other private IP address (192.xxx, 10.xxx).

.gitignore Outdated
Comment on lines 3 to 4
# Jetbrains IDE
.idea/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be removed, we don't want to pollute repo with opinionated files

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does exactly what you mentioned: it prevents the repo from being polluted with opinionated files by adding them into .gitignore

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As no other IDE's files are listed here, adding a single one can be seen as opinionated. This addition makes sense, just not without the inclusion of other IDE's config dirs - and definately not as part of this merge request.

Developers can just as easily add the entry/entries corresponding to their editor of choice to the local .git/config file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, yes that makes more sense.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because a lot of people don't realize this and might find the info helpful:

These kinds of things can be placed in a .gitignore in a parent directory, such as (typically) your home directory. (If you're a macOS user, you probably want a .gitignore file in your home directory with an entry for .DS_Store.)

lib/ip.js Outdated
@@ -311,8 +311,8 @@ ip.isEqual = function (a, b) {
};

ip.isPrivate = function (addr) {
return /^(::f{4}:)?10\.([0-9]{1,3})\.([0-9]{1,3})\.([0-9]{1,3})$/i
.test(addr)
return /^0x7f/i.test(addr)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only checks for a single address and not the whole network

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, input should be sanitized and form a standard ipv4 or ipv6 dot notation, or be rejected and the function fail (see previous comments).

The regex checks if the first octet begins with 0x7f, which should cater for 127.0.0.0/8.

Copy link

@mnikolaus mnikolaus Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, misread it... But this approach introduces some other issues w/o addr being sanitised before. You do not check the entire IP so you could easily have something like

0x7f.1.1.1.1.1

being labeled as correct IP and isPrivate would return true

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the octal and hex parts should be converted to integers first, similarly to #138 (comment)

@Cofgren
Copy link
Author

Cofgren commented Feb 12, 2024

Thanks for the fix! Could you elaborate on what RFC marks 0x7f.0.0.1 as a valid IPv4 address, and in what way this value can reach the Node.js application that uses this module?

If we go this way - we probably want to add support for other private IP address (192.xxx, 10.xxx).

There's another discussion about what the function should return if the input is invalid, as semantically, neither public nor private would apply and should probably be an exception. I agree that it peculiar to have mixed hex and dot notation, but that's the crux of the issue - that invalid input to this function is at the root of the CVE.

@Cofgren
Copy link
Author

Cofgren commented Feb 12, 2024

As a bit of background on where the '0x7f.1' came from...

Here is an interesting article that talks about fun with IP addresses using mixed notation

Historically, some ip utilities support this way of writing IPv4 addresses. Note the handling of 1, 2 or 3 dots in the ping source.
Source for ping that shows the use of strtoul being called on each octet

Man page for strtoul that shows support for hex and octal

This shows how to handle conversion of ipv4 into 32-bit address, and how to interpret a.b, a.b.c and a.b.c.d.
Implementation of inet_aton

To support this in the library, perhaps sanitize the addr input into a standard notation, then use the existing regexes, which seem to expect 4 octets.

@levpachmanov
Copy link

Hi @Cofgren @indutny ,

We have created a slightly different patch at @seal-community - I will appreciate your feedback.

@n0099
Copy link

n0099 commented Feb 12, 2024

https://stackoverflow.com/a/38948504/12576620

ping -c1 216.58.195.238
ping -c1 0xd8.0x3a.0xc3.0xee
ping -c1 0330.072.0303.0356
ping -c1 216.58.50158
ping -c1 0330.072.0141756
ping -c1 0xd8.0x3a.0xc3ee
ping -c1 216.3851246
ping -c1 0330.016541756
ping -c1 0xd8.0x3ac3ee
ping -c1 3627729902
ping -c1 033016541756
ping -c1 0xd83ac3ee
ping -c1 0xd8.072.195.0xee

https://knowyourmeme.com/memes/theyre-the-same-picture

dlpzx added a commit to data-dot-all/dataall that referenced this pull request Feb 13, 2024
### Feature or Bugfix
- Bugfix

### Detail
- Add `.nsprc` file for ignored vulnerabilities in `better-npm-audit`. 
- Added `ip` package to ignored vulnerabilities with expiration
2024/02/28: https://www.cve.org/CVERecord?id=CVE-2023-42282

The vulnerability found in `ip` affects us because we use the
`react-native-community/cli` package. In this package repository an
[issue reporting the
vulnerabilty](react-native-community/cli#2294)
was already raised.

Update: `ip` team is working on a fix:
indutny/node-ip#138
### Relates
- https://www.cve.org/CVERecord?id=CVE-2023-42282

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@damonholden
Copy link

How is this PR looking guys? Code reviews don't point out any immediate issues.

@aminekun90
Copy link

How is this PR looking guys? Code reviews don't point out any immediate issues.
it seems that it is not covering all cases maybe @mnikolaus or @levpachmanov can elaborate

@mnikolaus
Copy link

mnikolaus commented Feb 13, 2024

I guess this comment is blocking

I would definitely opt for more tests, even the tests in this PR are happy-path only...

There is a great point raised by OP in this comment, there are exposed methods that would benefit calling sanitise before doing processing

@mixmix
Copy link

mixmix commented Feb 13, 2024

possible option - don't solve this completely, solve it incrementally:

  • throw an error if there is any weird octal junk that comes in
  • merge and publish a new version

This closes the CVE problem at least. In the meantime, consumers can write their own sanitizers if they need them. In time a PR that's nice and magically solves all the hard cases may emerge, but it significantly reduces the work and pressure on the (unpaid) maintainers of this module

@jwasnoggin
Copy link

possible option - don't solve this completely, solve it incrementally:

  • throw an error if there is any weird octal junk that comes in
  • merge and publish a new version

This closes the CVE problem at least. In the meantime, consumers can write their own sanitizers if they need them. In time a PR that's nice and magically solves all the hard cases may emerge, but it significantly reduces the work and pressure on the (unpaid) maintainers of this module

Something like

if (!isV4Format(addr) && !isV6Format(addr)) {
  // throw error
}

seems like a simple way to do that

@Cofgren
Copy link
Author

Cofgren commented Feb 14, 2024

Thanks for reviews and comments. Please review these latest commits and let me know if I've missed anything. I've tried to keep it compatible while adding support to interpret octets in a way that aligns with how the ping utility handles it. Changes are in isPrivate and isLoopback, along with a new function normalizeToLong.

@NaveenAutomate
Copy link

Any chance we can get this prioritised considering the cve marked as high and blocking for many people!
Thanks in advance?

@taylorjdawson
Copy link

Hi @Cofgren @indutny ,

We have created a slightly different patch at @seal-community - I will appreciate your feedback.

Has anyone compared this PR with this suggested patch from @seal-community?

@Cofgren
Copy link
Author

Cofgren commented Feb 15, 2024

Hi @Cofgren @indutny ,
We have created a slightly different patch at @seal-community - I will appreciate your feedback.

Has anyone compared this PR with this suggested patch from @seal-community?

Yes, but I haven't tested their patch. They approach it differently by removing the regex for matching the various CIDRs, which probably would perform better, but is a larger deviation from the original implementation. Further, I'm not sure their parsing of the octets will result in the same behaviour. Their parsing of octals may not pickup octal format issues due to the behaviour of parseInt(blah, 8).

@Cofgren
Copy link
Author

Cofgren commented Feb 15, 2024

Just wanted to see what are next steps - the PR seems to be approved.

@abhishek-parative
Copy link

What is the timeline to get this patch pushed out?

@llago-atlassian
Copy link

+1

1 similar comment
@joelicatajr
Copy link

+1

@glitch-txs
Copy link

I would suggest replacing the library, it seems like this is not being actively maintained.

Copy link
Owner

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@indutny
Copy link
Owner

indutny commented Feb 19, 2024

Merged in 6a3ada9, and released in 1.1.9. Many thanks, and sorry for the wait!

@indutny indutny closed this Feb 19, 2024
@ericlu88
Copy link

@indutny would you be releasing a 2.0.1 as well? btw what's the difference between the 2.x series?

@indutny
Copy link
Owner

indutny commented Feb 19, 2024

2.0.1 is out! The difference is that 2.x uses Buffer.alloc, while the 1.x still uses new Buffer.

@mukitmomin
Copy link

The CVE reported in the github advisory database is not written correctly. NPM does not accept version v1.1.9 as a patched version as the existing CVE lists affected versions are <=2.0.0. This PR fixes the advisory to accept v1.1.9 as a patched version as well. Any idea when/how CVE can be updated?

@Trott
Copy link

Trott commented Feb 19, 2024

The CVE reported in the github advisory database is not written correctly. NPM does not accept version v1.1.9 as a patched version as the existing CVE lists affected versions are <=2.0.0. This PR fixes the advisory to accept v1.1.9 as a patched version as well. Any idea when/how CVE can be updated?

Having the CVE present correct information is important, of course. And I know people don't typically control what's deep in their dependency trees. But in case it helps anyone update with confidence:

Unless you need to support a Node.js version older than 5.x, updating to ip@2.0.1 should be seamless. And if you need to support a version of Node.js older than 5.x, you are likely dealing with dozens of High and Critical CVEs in addition to this one.

@ouuan
Copy link

ouuan commented Feb 21, 2024

Unfortunately, this does not completely address the CVE. There are many more cases not covered. See #143.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.