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

LGTM.com issues reported against Nmap #1834

Closed
guyharris opened this issue Nov 19, 2019 · 4 comments
Closed

LGTM.com issues reported against Nmap #1834

guyharris opened this issue Nov 19, 2019 · 4 comments

Comments

@guyharris
Copy link

@guyharris guyharris commented Nov 19, 2019

Originally mentioned in passing in #1827, LGTM.com has scanned Nmap and found some issues.

(Posted to, among other things, note that LGTM.com exists and is scanning at least some code in Nmap.)

@cldrn

This comment has been minimized.

Copy link
Member

@cldrn cldrn commented Nov 19, 2019

Thanks for the report. I am aware of this and we are working on some patches. We have confirmed the integer overflow in Nping and we will commit a patch soon.

@dmiller-nmap

This comment has been minimized.

Copy link

@dmiller-nmap dmiller-nmap commented Nov 22, 2019

Related commits:

nmap-bot pushed a commit that referenced this issue Dec 25, 2019
@dmiller-nmap

This comment has been minimized.

Copy link

@dmiller-nmap dmiller-nmap commented Dec 27, 2019

Since this issue was filed:

  • All "Security"-filter issues have been addressed apart from 3 multiplication overflows in Nping. Two of those we believe to be a single false positive, which we have reported as Semmle/ql#2561.
  • 31 alerts have been removed, including all "Error"-level alerts.
  • Our code quality score has increased from "C" (Python: B, C/C++: C) to "B" (Python: A, C/C++: B)
nmap-bot pushed a commit that referenced this issue Dec 29, 2019
nmap-bot pushed a commit that referenced this issue Dec 29, 2019
Our implementation of vsnprintf for systems where it is missing did not
correctly return a negative value on error, instead returning the size
passed in. We got this code from tcpdump/libpcap, and it was wrong
there, too, though their latest master branch has removed it in favor of
requiring a C99 compiler (C99 guarantees vsnprintf).

This should remove a LGTM code analysis finding (See #1834) of
cpp/constant-comparison in Ncat because we were checking for a negative
return from Snprintf, which would never occur.
nmap-bot pushed a commit that referenced this issue Dec 29, 2019
nmap-bot pushed a commit that referenced this issue Dec 29, 2019
nmap-bot pushed a commit that referenced this issue Dec 29, 2019
nmap-bot pushed a commit that referenced this issue Dec 29, 2019
nmap-bot pushed a commit that referenced this issue Dec 30, 2019
I18N installs the `_` function into the builtin namespace, so it looks
like it's unused when it's not. #1834
nmap-bot pushed a commit that referenced this issue Dec 30, 2019
nmap-bot pushed a commit that referenced this issue Jan 1, 2020
nmap-bot pushed a commit that referenced this issue Jan 2, 2020
I added this a long time ago, and decided to check it. In fact,
newstrlen is used to calculate newstrend, and each section of the
template is checked to ensure it does not go past newstrend, so the
intent is met and the length is not exceeded. I still think it could be
written more clearly, but it's good for now. #1834
nmap-bot pushed a commit that referenced this issue Jan 2, 2020
As the FIXME comment had said, looping over every integer up to maxfd is
inefficient, especially if FDs are not continuous. This change has the
added benefit of skipping a call to get_fdinfo(), which also loops over
all the client FDs looking for a particular value. Unlikely to be a huge
performance gain, but the code is cleaner. #1834 - FIXME comment.
@dmiller-nmap

This comment has been minimized.

Copy link

@dmiller-nmap dmiller-nmap commented Jan 15, 2020

Nmap is now at "A+" rating for C/C++ and Python, with only 25 open issues. I have added badges to the README.md that will hopefully serve as a reminder to use LGTM.com to find issues in the future, and I may decide to turn on continuous integration for pull requests, too. For now, I consider this issue resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.