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

Out-of-bounds references due to hacks that try to figure out if a string is in the local code page or in UTF-16LE #149

Open
guyharris opened this issue Apr 27, 2019 · 3 comments
Open

Out-of-bounds references due to hacks that try to figure out if a string is in the local code page or in UTF-16LE #149

guyharris opened this issue Apr 27, 2019 · 3 comments

Comments

@guyharris
Copy link
Contributor

WinPcap 4.1.1 added a hack to the adapter open path to work around pcap_lookupdev() returning UTF-16LE strings rather than ASCII strings for device names.

The hack attempts to check whether the device name string is ASCII (local code page) or UTF-16LE by checking whether the first byte is non-zero and the second byte is zero. If that's the case, it's treated as a UTF-16LE null-terminated string.

Unfortunately, a one-character ASCII string can't be distinguished from the first byte of a UTF-16LE string, and if you mis-identify a one-character ASCII string as a UTF-16LE string, you'll go past the end of the string when processing it as a UTF-16LE string.

The comment from WinPcap 4.1.1 is

	 * This wonderful hack is needed because pcap_lookupdev still returns
	 * unicode strings, and it's used by windump when no device is specified
	 * in the command line

Tcpdump, however, uses pcap_findalldevs() if it's available, picking the first interface from the list, so this bug no longer affects it, and pcap_lookupdev() is now marked as deprecated. It always returns ASCII device names, so programs using pcap_findalldevs() don't require this workaround.

If binary compatibility with programs using pcap_lookupdev() to find a default interface can be dropped in Npcap, the hack can be dropped as well.
@guyharris guyharris changed the title Unfixable pcap_create() out-of-bounds reference due to a workaround hack introduced in WinPcap 4.x Unfixable pcap_create() out-of-bounds reference due to a workaround hack introduced in WinPcap 4.1.1 Apr 27, 2019
@guyharris
Copy link
Contributor Author

And PacketOpenAdapter(), dating back to WinPcap 3.1 (and possibly earlier), has the same "if the second octet is zero, assume it's a UTF-16LE string" hack.

Character encodings need some cleanup in libpcap-for-Windows - there should be versions of the API that uniformly use the local code page for strings and that uniformly use UTF-8 for strings, with a define such as PCAP_UTF_8 that, if defined, causes the UTF-8 version of the API to be used, with the local code page version being used otherwise. That way, old programs will still see local code page APIs, and new programs either coming from UNIXland (and making no code page assumptions) or from Windows (and wanting to use full Unicode) can get UTF-8 support.

I'm going to look at implementing that. Is the packet32.dll API sufficiently important enough to need to be stable enough that some of its routines may need similar treatment?

@guyharris guyharris changed the title Unfixable pcap_create() out-of-bounds reference due to a workaround hack introduced in WinPcap 4.1.1 Out-of-bounds reference due to hacks that try to figure out if a string is in the local code page or in UTF-16LE Apr 28, 2019
@guyharris guyharris changed the title Out-of-bounds reference due to hacks that try to figure out if a string is in the local code page or in UTF-16LE Out-of-bounds references due to hacks that try to figure out if a string is in the local code page or in UTF-16LE Apr 28, 2019
@dmiller-nmap
Copy link
Contributor

Replying to your last question first: no, the Packet API is not officially documented and supported in such a way as to require stability from one release to the next. Npcap's official API is the libpcap API, so if libpcap drops pcap_lookupdev, then so will Npcap. The only thing we really need to be sure of is that whichever version of libpcap we ship will work with the version of the Packet API we ship in that particular release.

Somewhat related: I noticed that our code to translate "NPF" names to "NPCAP" names is ASCII-only (or more properly, cannot handle embedded 0 bytes, so UTF-8 would be ok), but it's being called before that problematic translation of Unicode to ASCII. I'm moving it later, which might improve support in cases where the name is passed in as Unicode, but of course we need a better solution in the long run.

dmiller-nmap referenced this issue Jun 24, 2019
@bonsaiviking
Do adapter name translation after converting to ASCII. See nmap/nmap#…

Verified

This commit was signed with the committer’s verified signature.
rht
GPG key ID: 1E76EA93A9B3D3E2
Verified
Learn about vigilant mode
c032935 
…1575
@guyharris
Copy link
Contributor Author

And PacketOpenAdapter(), dating back to WinPcap 3.1 (and possibly earlier), has the same "if the second octet is zero, assume it's a UTF-16LE string" hack.

Character encodings need some cleanup in libpcap-for-Windows - there should be versions of the API that uniformly use the local code page for strings and that uniformly use UTF-8 for strings, with a define such as PCAP_UTF_8 that, if defined, causes the UTF-8 version of the API to be used, with the local code page version being used otherwise. That way, old programs will still see local code page APIs, and new programs either coming from UNIXland (and making no code page assumptions) or from Windows (and wanting to use full Unicode) can get UTF-8 support.

Done in the master branch, but it's not done with a #define, it's done by an argument to a new pcap_init() routine (which also initializes Winsock on Windows; the routine exists on UN*X as well, so calls to it only have to be #ifdeffed to avoid calling it if you have an older pcap, so you don't have to do #ifdefs for initializing Winsock for WinPcap/Npcap).

@fyodor fyodor transferred this issue from nmap/nmap May 20, 2020
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

No branches or pull requests
2 participants
@guyharris @dmiller-nmap