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

Switch to immediate mode #1291

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@djcater

djcater commented Aug 2, 2018

WIP for #34.

This avoids false packet drops caused by libpcap buffering packets before returning them, making Nmap think that there is no response within its expected round trip timeout, leading to additional probes sent as retries. When the buffered packets then are returned, Nmap assumes that there were packet drops, due to getting responses "after" its retry probes, but not from the first probe (in reality, no packets were lost).

This is based on code seen in here: #34 (comment)

It's just a quick patch to show the change. It doesn't implement error checking on pcap_activate, and doesn't check to see if pcap_set_immediate_mode is supported in the case of OS-provided libpcap.

Update netutil.cc
WIP for #34.

This avoids false packet drops caused by libpcap buffering packets before returning them, making Nmap think that there is no response within its expected round trip timeout, leading to additional probes sent as retries. When the buffered packets then are returned, Nmap assumes that there were packet drops, due to getting responses "after" its retry probes, but not from the first probe (in reality, no packets were lost).

This is based on code seen in here: #34 (comment)

It's just a quick patch to show the change. It doesn't implement error checking on pcap_activate, and doesn't check to see if pcap_set_immediate_mode is supported in the case of OS-provided libpcap.

@djcater djcater changed the title from Update netutil.cc to Switch to immediate mpde Aug 2, 2018

@djcater djcater changed the title from Switch to immediate mpde to Switch to immediate mode Aug 2, 2018

@mpontillo

This comment has been minimized.

Show comment
Hide comment
@mpontillo

mpontillo Aug 2, 2018

I'm happy to see some activity on this. (By the way, is nmap development active on GitHub via pull requests? I see that, at least when I initially wrote up that PoC code, the official tree was being maintained Subversion; are patches being integrated via mailing list discussion? Either way, this is a nice place to comment on the request, so thank you.)

I wasn't sure if libpcap would always have the newly-used function calls (such as older versions, or versions on other platforms), so I wondered if some amount of #ifdef checking might be required for the patch to make it upstream. Guy Harris talks about pcap_create()/pcap_activate() being the "new way" in a mailing list post from 2012, but I don't know if people are still using versions of libpcap older than that, such as using a new version of nmap on a legacy platform.

Regarding error handling, I agree it will need to be split into (at least) two parts now, whereas the original code just had a do { } loop to retry around pcap_open_live. Using pcap_create instead means that error checking needs to happen both on pcap_create() and on pcap_activate(), and possibly the functions used to set parameters. From man pcap_create:

RETURN VALUE
   pcap_create()  returns  a  pcap_t * on success and NULL on failure.
   If NULL is returned, errbuf is filled in with an appropriate  error
   message.    errbuf   is  assumed  to  be  able  to  hold  at  least
   PCAP_ERRBUF_SIZE chars.

And from man pcap_activate:

RETURN VALUE
   pcap_activate()  returns  0 on success without warnings, a non-zero
   positive value on success with warnings, and a  negative  value  on
   error.   A  non-zero  return  value indicates what warning or error
   condition occurred.

   The possible warning values are:

   PCAP_WARNING_PROMISC_NOTSUP
          Promiscuous mode  was  requested,  but  the  capture  source
          doesn't support promiscuous mode.

   PCAP_WARNING_TSTAMP_TYPE_NOTSUP
          The    time    stamp    type   specified   in   a   previous
          pcap_set_tstamp_type() call isn't supported by  the  capture
          source (the time stamp type is left as the default),

   PCAP_WARNING
          Another   warning   condition   occurred;  pcap_geterr()  or
          pcap_perror() may be called with p as an argument  to  fetch
          or display a message describing the warning condition.

   The possible error values are:

   PCAP_ERROR_ACTIVATED
          The handle has already been activated.

   PCAP_ERROR_NO_SUCH_DEVICE
          The  capture  source  specified  when the handle was created
          doesn't exist.

   PCAP_ERROR_PERM_DENIED
          The process doesn't have  permission  to  open  the  capture
          source.

   PCAP_ERROR_PROMISC_PERM_DENIED
          The  process  has  permission to open the capture source but
          doesn't have permission to put it into promiscuous mode.

   PCAP_ERROR_RFMON_NOTSUP
          Monitor mode was specified but the  capture  source  doesn't
          support monitor mode.

   PCAP_ERROR_IFACE_NOT_UP
          The capture source device is not up.

   PCAP_ERROR
          Another  error occurred.  pcap_geterr() or pcap_perror() may
          be called with p as an argument to fetch or display  a  mes‐
          sage describing the error.

   If   PCAP_WARNING_PROMISC_NOTSUP,   PCAP_ERROR_NO_SUCH_DEVICE,   or
   PCAP_ERROR_PERM_DENIED is returned, pcap_geterr() or  pcap_perror()
   may  be called with p as an argument to fetch or display an message
   giving additional details about the problem that  might  be  useful
   for debugging the problem if it's unexpected.

   Additional  warning  and  error codes may be added in the future; a
   program should check for positive, negative, and zero return codes,
   and  treat  all  positive return codes as warnings and all negative
   return codes as errors.  pcap_statustostr() can be called,  with  a
   warning or error code as an argument, to fetch a message describing
   the warning or error code.

What isn't clear to me is why the do { } loop was used. Since pcap_open_live() is essentially a combination of creation and activation, it isn't clear which error condition(s) might require retrying.

I would guess that it would be better if the do {} loop were moved to the pcap_activate() call, simply because the only obvious error that a retry would help with is PCAP_ERROR_IFACE_NOT_UP. Without knowing why the loop was there to begin with, though, it's difficult to say.

The examples of error handling in this test program might be helpful, too.

mpontillo commented Aug 2, 2018

I'm happy to see some activity on this. (By the way, is nmap development active on GitHub via pull requests? I see that, at least when I initially wrote up that PoC code, the official tree was being maintained Subversion; are patches being integrated via mailing list discussion? Either way, this is a nice place to comment on the request, so thank you.)

I wasn't sure if libpcap would always have the newly-used function calls (such as older versions, or versions on other platforms), so I wondered if some amount of #ifdef checking might be required for the patch to make it upstream. Guy Harris talks about pcap_create()/pcap_activate() being the "new way" in a mailing list post from 2012, but I don't know if people are still using versions of libpcap older than that, such as using a new version of nmap on a legacy platform.

Regarding error handling, I agree it will need to be split into (at least) two parts now, whereas the original code just had a do { } loop to retry around pcap_open_live. Using pcap_create instead means that error checking needs to happen both on pcap_create() and on pcap_activate(), and possibly the functions used to set parameters. From man pcap_create:

RETURN VALUE
   pcap_create()  returns  a  pcap_t * on success and NULL on failure.
   If NULL is returned, errbuf is filled in with an appropriate  error
   message.    errbuf   is  assumed  to  be  able  to  hold  at  least
   PCAP_ERRBUF_SIZE chars.

And from man pcap_activate:

RETURN VALUE
   pcap_activate()  returns  0 on success without warnings, a non-zero
   positive value on success with warnings, and a  negative  value  on
   error.   A  non-zero  return  value indicates what warning or error
   condition occurred.

   The possible warning values are:

   PCAP_WARNING_PROMISC_NOTSUP
          Promiscuous mode  was  requested,  but  the  capture  source
          doesn't support promiscuous mode.

   PCAP_WARNING_TSTAMP_TYPE_NOTSUP
          The    time    stamp    type   specified   in   a   previous
          pcap_set_tstamp_type() call isn't supported by  the  capture
          source (the time stamp type is left as the default),

   PCAP_WARNING
          Another   warning   condition   occurred;  pcap_geterr()  or
          pcap_perror() may be called with p as an argument  to  fetch
          or display a message describing the warning condition.

   The possible error values are:

   PCAP_ERROR_ACTIVATED
          The handle has already been activated.

   PCAP_ERROR_NO_SUCH_DEVICE
          The  capture  source  specified  when the handle was created
          doesn't exist.

   PCAP_ERROR_PERM_DENIED
          The process doesn't have  permission  to  open  the  capture
          source.

   PCAP_ERROR_PROMISC_PERM_DENIED
          The  process  has  permission to open the capture source but
          doesn't have permission to put it into promiscuous mode.

   PCAP_ERROR_RFMON_NOTSUP
          Monitor mode was specified but the  capture  source  doesn't
          support monitor mode.

   PCAP_ERROR_IFACE_NOT_UP
          The capture source device is not up.

   PCAP_ERROR
          Another  error occurred.  pcap_geterr() or pcap_perror() may
          be called with p as an argument to fetch or display  a  mes‐
          sage describing the error.

   If   PCAP_WARNING_PROMISC_NOTSUP,   PCAP_ERROR_NO_SUCH_DEVICE,   or
   PCAP_ERROR_PERM_DENIED is returned, pcap_geterr() or  pcap_perror()
   may  be called with p as an argument to fetch or display an message
   giving additional details about the problem that  might  be  useful
   for debugging the problem if it's unexpected.

   Additional  warning  and  error codes may be added in the future; a
   program should check for positive, negative, and zero return codes,
   and  treat  all  positive return codes as warnings and all negative
   return codes as errors.  pcap_statustostr() can be called,  with  a
   warning or error code as an argument, to fetch a message describing
   the warning or error code.

What isn't clear to me is why the do { } loop was used. Since pcap_open_live() is essentially a combination of creation and activation, it isn't clear which error condition(s) might require retrying.

I would guess that it would be better if the do {} loop were moved to the pcap_activate() call, simply because the only obvious error that a retry would help with is PCAP_ERROR_IFACE_NOT_UP. Without knowing why the loop was there to begin with, though, it's difficult to say.

The examples of error handling in this test program might be helpful, too.

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Aug 2, 2018

Guy Harris talks about pcap_create()/pcap_activate() being the "new way" in a mailing list post from 2012, but I don't know if people are still using versions of libpcap older than that,

Libpcap 1.0.0 was the first release with pcap_create() and pcap_activate(), and it was released in October 2008.

guyharris commented Aug 2, 2018

Guy Harris talks about pcap_create()/pcap_activate() being the "new way" in a mailing list post from 2012, but I don't know if people are still using versions of libpcap older than that,

Libpcap 1.0.0 was the first release with pcap_create() and pcap_activate(), and it was released in October 2008.

@mpontillo

This comment has been minimized.

Show comment
Hide comment
@mpontillo

mpontillo Aug 2, 2018

Thanks Guy; now that it's nearly a decade later, I personally feel that it would be sane for nmap to drop support for libpcap versions less than 1.0.0. (But I'm not sure how the community should decide that.)

mpontillo commented Aug 2, 2018

Thanks Guy; now that it's nearly a decade later, I personally feel that it would be sane for nmap to drop support for libpcap versions less than 1.0.0. (But I'm not sure how the community should decide that.)

@mpontillo

This comment has been minimized.

Show comment
Hide comment
@mpontillo

mpontillo Aug 2, 2018

Ah, I just noticed @djcater's comment on the bug that points out that pcap_set_immediate_mode() was introduced in libpcap-1.5.0 in 2013. (I guess it would have been nice to know that from reading the man page.) That's still ages ago in software development time, but it strengthens the argument for an #if of some sort in case the function call just isn't there on some older, non-updated platform or abandoned port of libpcap.

mpontillo commented Aug 2, 2018

Ah, I just noticed @djcater's comment on the bug that points out that pcap_set_immediate_mode() was introduced in libpcap-1.5.0 in 2013. (I guess it would have been nice to know that from reading the man page.) That's still ages ago in software development time, but it strengthens the argument for an #if of some sort in case the function call just isn't there on some older, non-updated platform or abandoned port of libpcap.

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Aug 3, 2018

So, on systems without pcap_set_immediate_mode():

  • If it's a system using BPF, for which read "a system with <net/bpf.h>, if that header defines BIOCIMMEDIATE, you can, after pcap_activate() returns successfully, do a BIOCIMMEDIATE ioctl to turn immediate mode on;

  • if it's a Linux system, if libpcap doesn't have pcap_set_immediate_mode() it also doesn't support TPACKET_V3, so nothing needs to be done;

  • If it's a Windows system, after pcap_activate() returns successfully, use pcap_setmintocopy() and set the minimum-to-copy to 0;

  • if it's a system running SunOS 3.x, SunOS 4.x, or Tru64 UNIX, see the appropriate pcap-XXX.c file for what you need to do.

guyharris commented Aug 3, 2018

So, on systems without pcap_set_immediate_mode():

  • If it's a system using BPF, for which read "a system with <net/bpf.h>, if that header defines BIOCIMMEDIATE, you can, after pcap_activate() returns successfully, do a BIOCIMMEDIATE ioctl to turn immediate mode on;

  • if it's a Linux system, if libpcap doesn't have pcap_set_immediate_mode() it also doesn't support TPACKET_V3, so nothing needs to be done;

  • If it's a Windows system, after pcap_activate() returns successfully, use pcap_setmintocopy() and set the minimum-to-copy to 0;

  • if it's a system running SunOS 3.x, SunOS 4.x, or Tru64 UNIX, see the appropriate pcap-XXX.c file for what you need to do.

@djcater

This comment has been minimized.

Show comment
Hide comment
@djcater

djcater Aug 4, 2018

Thanks again for the detailed suggestions @guyharris.

Just below these changes there is already a line for Windows which does:

pcap_setmintocopy(pt, 1);

For the other parts, hopefully someone else can help out and make those changes.

djcater commented Aug 4, 2018

Thanks again for the detailed suggestions @guyharris.

Just below these changes there is already a line for Windows which does:

pcap_setmintocopy(pt, 1);

For the other parts, hopefully someone else can help out and make those changes.

@dmiller-nmap

This comment has been minimized.

Show comment
Hide comment
@dmiller-nmap

dmiller-nmap Aug 20, 2018

This is great! I'm going to work on merging this. I'm not concerned about the retry loop, since that was added back in Nmap 4.02ALPHA1 to work around "a rare bug on Windows in which the pcap_open_live() fails for unknown reasons." Since we now use Npcap and are responsible for maintaining it, we would much rather discover the root cause (if it still exists) and fix it there.

Thanks everyone for your research and guidance on this issue!

dmiller-nmap commented Aug 20, 2018

This is great! I'm going to work on merging this. I'm not concerned about the retry loop, since that was added back in Nmap 4.02ALPHA1 to work around "a rare bug on Windows in which the pcap_open_live() fails for unknown reasons." Since we now use Npcap and are responsible for maintaining it, we would much rather discover the root cause (if it still exists) and fix it there.

Thanks everyone for your research and guidance on this issue!

Add TODO comments.
Add some comments for open questions, to ensure they're thought about before accepting this change.
@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Aug 20, 2018

I guess it would have been nice to know that from reading the man page.

I.e., have an "introduced in" section in the libpcap man pages for particular functions (or in the pcap(3pcap) overall man page, or both)?

guyharris commented Aug 20, 2018

I guess it would have been nice to know that from reading the man page.

I.e., have an "introduced in" section in the libpcap man pages for particular functions (or in the pcap(3pcap) overall man page, or both)?

@mpontillo

This comment has been minimized.

Show comment
Hide comment
@mpontillo

mpontillo Aug 20, 2018

@guyharris it seems we're getting a little off-topic for this PR. ;-) But I think it would be nice-to-have in the individual function man pages, that way when I'm looking at the documentation for a specific call I want to use, I can be sure it will be available across all the platforms I want to support. (I see there also a BACKWARDS COMPATIBILITY section in the 3pcap man page, which could also be a nice place for that kind of information - but that looks a little more general than information like which release a function call was introduced in.)

I think this PR illustrates two challenges with cross-platform development with libpcap; first, if you're targeting multiple versions of libpcap, it's difficult to know the minimum required libpcap version for support. Second, even if you're fairly certain you have the minimum-required version of libpcap, there are additional platform-specific steps needed for certain tasks (such as enabling immediate mode). It would be nice if libpcap could abstract all those aspects into the single pcap_set_immediate_mode() call.

Finally, I just wanted to say that I appreciate the hard work (and public contributions) from everyone in this thread! I hope my comments are coming across as constructive feedback and not pedantic complaints. ;-)

mpontillo commented Aug 20, 2018

@guyharris it seems we're getting a little off-topic for this PR. ;-) But I think it would be nice-to-have in the individual function man pages, that way when I'm looking at the documentation for a specific call I want to use, I can be sure it will be available across all the platforms I want to support. (I see there also a BACKWARDS COMPATIBILITY section in the 3pcap man page, which could also be a nice place for that kind of information - but that looks a little more general than information like which release a function call was introduced in.)

I think this PR illustrates two challenges with cross-platform development with libpcap; first, if you're targeting multiple versions of libpcap, it's difficult to know the minimum required libpcap version for support. Second, even if you're fairly certain you have the minimum-required version of libpcap, there are additional platform-specific steps needed for certain tasks (such as enabling immediate mode). It would be nice if libpcap could abstract all those aspects into the single pcap_set_immediate_mode() call.

Finally, I just wanted to say that I appreciate the hard work (and public contributions) from everyone in this thread! I hope my comments are coming across as constructive feedback and not pedantic complaints. ;-)

@dmiller-nmap

This comment has been minimized.

Show comment
Hide comment
@dmiller-nmap

dmiller-nmap Aug 20, 2018

I've committed the initial portion of this as 2 commits:

  1. updating configure scripts to require pcap_create (and therefore libpcap 1.0.0 or greater. We already had a version number check for 0.9.4 or greater.)
  2. switching to pcap_create and pcap_activate with error checking, but without adding pcap_set_immediate.

When I get back to this, I'll add the appropriate code workarounds for immediate mode for all platforms and credit the parties involved in the changelog.

dmiller-nmap commented Aug 20, 2018

I've committed the initial portion of this as 2 commits:

  1. updating configure scripts to require pcap_create (and therefore libpcap 1.0.0 or greater. We already had a version number check for 0.9.4 or greater.)
  2. switching to pcap_create and pcap_activate with error checking, but without adding pcap_set_immediate.

When I get back to this, I'll add the appropriate code workarounds for immediate mode for all platforms and credit the parties involved in the changelog.

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Aug 20, 2018

it seems we're getting a little off-topic for this PR. ;-) But I think it would be nice-to-have in the individual function man pages ...

Then the discussion should be moved by filing a libpcap issue.

Second, even if you're fairly certain you have the minimum-required version of libpcap, there are additional platform-specific steps needed for certain tasks (such as enabling immediate mode). It would be nice if libpcap could abstract all those aspects into the single pcap_set_immediate_mode() call.

Libpcap does abstract that particular platform-specific issue - enabling immediate mode - into pcap_set_immediate_mode() (and the per-platform/per-capture source pcap_activate() code).

The problem isn't that libpcap doesn't offer a way to request immediate mode, the problem is that libpcap didn't offer a way to request immediate mode before the 1.5.0 release, so any program or library that has to support older versions has to, if it's being built with a pre-1.5.0 release, duplicate what libpcap 1.5.0 does. Without a time machine, there's nothing we, the libpcap developers, can do to fix that. :-)

BTW, if you do have pcap_set_immediate_mode() in the version of libpcap for which you're building, I strongly advise you to use it unless you don't care whether you support Linux - earlier releases of libpcap didn't use TPACKET_V3, so there's nothing that needs to be done to get immediate notification and delivery of packets, but 1.5.0 does use TPACKET_V3 if the kernel (and build environment in which libpcap was built) supports it, and there's nothing you can do after the pcap_t has been activated to make it use TPACKET_V2 instead.

guyharris commented Aug 20, 2018

it seems we're getting a little off-topic for this PR. ;-) But I think it would be nice-to-have in the individual function man pages ...

Then the discussion should be moved by filing a libpcap issue.

Second, even if you're fairly certain you have the minimum-required version of libpcap, there are additional platform-specific steps needed for certain tasks (such as enabling immediate mode). It would be nice if libpcap could abstract all those aspects into the single pcap_set_immediate_mode() call.

Libpcap does abstract that particular platform-specific issue - enabling immediate mode - into pcap_set_immediate_mode() (and the per-platform/per-capture source pcap_activate() code).

The problem isn't that libpcap doesn't offer a way to request immediate mode, the problem is that libpcap didn't offer a way to request immediate mode before the 1.5.0 release, so any program or library that has to support older versions has to, if it's being built with a pre-1.5.0 release, duplicate what libpcap 1.5.0 does. Without a time machine, there's nothing we, the libpcap developers, can do to fix that. :-)

BTW, if you do have pcap_set_immediate_mode() in the version of libpcap for which you're building, I strongly advise you to use it unless you don't care whether you support Linux - earlier releases of libpcap didn't use TPACKET_V3, so there's nothing that needs to be done to get immediate notification and delivery of packets, but 1.5.0 does use TPACKET_V3 if the kernel (and build environment in which libpcap was built) supports it, and there's nothing you can do after the pcap_t has been activated to make it use TPACKET_V2 instead.

@mpontillo

This comment has been minimized.

Show comment
Hide comment
@mpontillo

mpontillo Aug 20, 2018

@guyharris, thanks for the clarification - I mistakenly interpreted your previous comment to mean that some versions of libpcap might not have a pcap_set_immediate_mode() call regardless of version.

mpontillo commented Aug 20, 2018

@guyharris, thanks for the clarification - I mistakenly interpreted your previous comment to mean that some versions of libpcap might not have a pcap_set_immediate_mode() call regardless of version.

nmap-bot pushed a commit that referenced this pull request Aug 20, 2018

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Aug 20, 2018

I mistakenly interpreted your previous comment to mean that some versions of libpcap might not have a pcap_set_immediate_mode() call regardless of version.

There's "version" and there's "version".

For "version" in the sense of "release", releases prior to 1.5.0 don't have it, and release 1.5.0 and later do have it.

For "version" in the sense of "something built by somebody who's downloaded the source", if somebody wanted to make a version of a 1.5.0 or later release that didn't have pcap_set_immediate_mode(), they'd have to manually edit pcap.c to remove it, which they probably won't do. (If they wanted to add it, that'd be a lot more work; they might as well just grab a current release and go with that.)

guyharris commented Aug 20, 2018

I mistakenly interpreted your previous comment to mean that some versions of libpcap might not have a pcap_set_immediate_mode() call regardless of version.

There's "version" and there's "version".

For "version" in the sense of "release", releases prior to 1.5.0 don't have it, and release 1.5.0 and later do have it.

For "version" in the sense of "something built by somebody who's downloaded the source", if somebody wanted to make a version of a 1.5.0 or later release that didn't have pcap_set_immediate_mode(), they'd have to manually edit pcap.c to remove it, which they probably won't do. (If they wanted to add it, that'd be a lot more work; they might as well just grab a current release and go with that.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment