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.

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

Jump to bottom

requests.utils function accepts invalid ip/cidr input #5131

Open
disconnect3d opened this issue Jul 3, 2019 · 9 comments · May be fixed by #6675
Open

requests.utils function accepts invalid ip/cidr input #5131

disconnect3d opened this issue Jul 3, 2019 · 9 comments · May be fixed by #6675

Comments

@disconnect3d
Copy link

disconnect3d commented Jul 3, 2019 •
edited
Loading

TLDR: On some libc implementations (e.g. glibc) the socket.inet_aton function parses IP strings trailed with whitespace and garbage, and this function is used by requests utility functions which if used externally, may cause bugs or/and security vulnerabilities.

An example of the issue can be seen below:
image

I have written a more detailed description of the socket.inet_aton's underlying problem on Python's bugtracker issue 37495. It is yet to be decided if this is going to be fixed in Python.

Expected Result

The requests.utils functions address_in_network, is_ipv4_address and is_valid_cidr should fail with invalid input.

Actual Result

Incorrect IP strings do not return an error and instead return as if the trailing garbage did not exist in the IP string.

Reproduction Steps

import requests

print(requests.utils.address_in_network('1.1.1.1 wtf', '1.1.1.1/24'))
print(requests.utils.is_ipv4_address('1.1.1.1 disconnect3d was here...'))
print(requests.utils.is_valid_cidr('1.1.1.1 obviously not but yes/24'))

System Information

The issue is related to libc implementation and has been tested on glibc 2.27 and 2.29.

It also occurs on MacOS which I am not sure if it is based on glibc.
@HJones82493 HJones82493 mentioned this issue Jul 10, 2019
Closed
@HJones82493
Copy link

PR #5135
I feel as though the call to the socket library is actually unnecessary here.

@disconnect3d
Copy link
Author

Ping @kennethreitz @Lukasa @sigmavirus24 @nateprewitt @slingamn can we finally get some love here and get this fixed?

This could probably also be marked as a security issue since if someone uses this in a security relevant context, they can get in trouble. (Although I doubt and hope that nobody does use those functions in such a way).

@sigmavirus24
Copy link
Contributor

These are not intended for use by others. The fact the language has no way of making things as private or internal means that we can't stop people from giving a function not intended for public use garbage or getting garbage out

@sigmavirus24
Copy link
Contributor

Also, @disconnect3d rather than demand free labor of others, you're welcome to submit a fix for this

@disconnect3d
Copy link
Author

These are not intended for use by others. The fact the language has no way of making things as private or internal means that we can't stop people from giving a function not intended for public use garbage or getting garbage out

The typical way to mark this is to prefix the name with underscore or two ;).

@sigmavirus24
Copy link
Contributor

Yeah and people still use it from this library so we stopped bothering. I've been maintaining this library for almost a decade now. Writing python for longer. I know what the convention is and I know people don't care

@HJones82493
Copy link

Is there an argument against using socket.inet_pton instead of socket.inet_aton? It seems like socket.inet_pton has had its implementation fixed to not accept the extra strings and could solve this whole problem? If there's historical reason to not use it in these contexts, that's fine, but it seems like a simple fix (I'd be more than willing to submit something for review for it) if not.

@vcapparelli vcapparelli linked a pull request Mar 27, 2024 that will close this issue
Open
@vcapparelli
Copy link

PR #6675
@disconnect3d , the socket implementation was replaced by ipaddress, fixing the bugs

@disconnect3d
Copy link
Author

disconnect3d commented Mar 28, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix utils functions with invalid ip/cidr inputs vcapparelli/requests
4 participants
@sigmavirus24 @disconnect3d @HJones82493 @vcapparelli