Skip to content

CVE-2023-42282 #138

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

CVE-2023-42282 #138

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f668b59
updated .gitignore
Feb 12, 2024
10b0e2f
lib: fixed CVE-2023-42282 and added unit test
Feb 12, 2024
e396dfd
lib: fixed CVE-2023-42282 and added unit test
Feb 12, 2024
a395cff
sanitized .gitignore
Feb 13, 2024
0b9a13d
normalize ipv4 addresses prior to regex
Feb 14, 2024
a1fea99
added 2130706433 (127.0.0.1) to isLoopback
Feb 14, 2024
990382e
handle long as input in isLoopback + more tests
Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
node_modules/
npm-debug.log
# Jetbrains IDE
.idea/
Copy link

@mnikolaus mnikolaus Feb 12, 2024

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

Copy link

@narek-haytyan narek-haytyan Feb 12, 2024

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

Copy link

@woutervanvliet woutervanvliet Feb 13, 2024

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.

Copy link

@narek-haytyan narek-haytyan Feb 13, 2024

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

@Trott Trott Feb 15, 2024

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.)

5 changes: 3 additions & 2 deletions lib/ip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

@mnikolaus mnikolaus Feb 12, 2024

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

@Cofgren Cofgren Feb 12, 2024

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 •
edited
Loading

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

@Cofgren Cofgren Feb 13, 2024

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)

|| /^(::f{4}:)?10\.([0-9]{1,3})\.([0-9]{1,3})\.([0-9]{1,3})$/i.test(addr)
|| /^(::f{4}:)?192\.168\.([0-9]{1,3})\.([0-9]{1,3})$/i.test(addr)
|| /^(::f{4}:)?172\.(1[6-9]|2\d|30|31)\.([0-9]{1,3})\.([0-9]{1,3})$/i
.test(addr)
Expand All @@ -331,6 +331,7 @@ ip.isPublic = function (addr) {
ip.isLoopback = function (addr) {
return /^(::f{4}:)?127\.([0-9]{1,3})\.([0-9]{1,3})\.([0-9]{1,3})/
.test(addr)
|| /^0x7f/i.test(addr)
|| /^fe80::1$/.test(addr)
|| /^::1$/.test(addr)
|| /^::$/.test(addr);
Expand Down
4 changes: 4 additions & 0 deletions test/api-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,10 @@ describe('IP library for node.js', () => {
assert.equal(ip.isPrivate('::1'), true);
assert.equal(ip.isPrivate('fe80::1'), true);
});

it('should correctly identify hexadecimal IP addresses like \'0x7f.1\' as private', () => {
assert.equal(ip.isPrivate('0x7f.1'), true);
});
});

describe('loopback() method', () => {
Expand Down