Identd: fix various issues #4872
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Per RFC 1413, The uniquely identifying tuple includes not only the ports, but also both addresses. If multiple connections happen to use the same local port number (which is possible if the addresses differ), the username of the first is returned for all, resulting in the wrong ident for all but the first. By not checking the connection address, the information becomes public. Because there is only relatively small number of local ports, and the remote ports are likely to be either 6667 or 6697, it becomes trivial to enumerate all the users. Co-Authored-By: Juerd Waalboer <juerd@tnx.nl>
We only respond once to data, then half-close the connection. Hence, we should only listen to a single data event as well, else if the remote doesn't stop sending data we keep trying to write to the closed write end of the pipe.
|
@Juerd I've added you as co-authored-by, hope that's ok but if not ping me and I'll amend the commit |
brunnre8
commented
May 9, 2024
| connection.socket.remotePort === fport && | ||
| connection.socket.localPort === lport && | ||
| socket.remoteAddress === connection.socket.remoteAddress && | ||
| socket.localAddress === connection.socket.localAddress |
There was a problem hiding this comment.
This ignores ipv4 in ipv6 handling...
Meaning if for some reason our ident server listens on ipv6 (dual stack) but the IRC connection is in ipv4 we will wrongly send an not found response.
Considering that you have to try rather hard to get to that edge case, I chose not to care.
|
Reto skribis 2024-05-09 10:06 (-0700):
@Juerd I've added you as co-authored-by, hope that's ok but if not ping me and I'll amend the commit
That is fine, thanks for confirming this with me.
|
MaxLeiter
approved these changes
May 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a bunch of sub optimal behavior from our ident server.
For one, it allows user enumeration which we don't really want and it doesn't clean up connections that don't send any data.
Fix that