Bug 236292 - sbin/ipfw doesn't allow returning packets with limit-source address
Summary: sbin/ipfw doesn't allow returning packets with limit-source address
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.0-STABLE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-ipfw mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-05 19:26 UTC by Dries Michiels
Modified: 2019-04-14 12:05 UTC (History)
1 user (show)

See Also:


Attachments
Proposed patch (for stable/12) (1.20 KB, patch)
2019-03-06 09:01 UTC, Andrey V. Elsukov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dries Michiels 2019-03-05 19:26:29 UTC
Hi,

After upgrading from source from r343710 to r344737 the behavior of limit-source address changed.

I have rules like this (which skip after my ipv4 NAT rule where a accept all rule is):

skipto 10000 ip4 from any to me 443 in recv em0 proto tcp limit src-addr 10

Altough after my upgrade, these packets don't get allowed out.
When I change the rule to the below one it works just fine.

skipto 10000 ip4 from any to me 443 in recv em0 proto tcp keep-state


I see the dynamic rule getting installed with LIMIT:
[/usr/src]$ sudo ipfw show -d |grep LIMIT
00000        2         120 (19s) LIMIT tcp 109.140.18.212 10087 <-> 141.135.72.71 443 :default
00000        3         180 (299s) LIMIT tcp 109.140.18.212 10087 <-> 141.135.72.71 443 :default
00000        3         180 (296s) LIMIT tcp 109.140.18.212 10087 <-> 141.135.72.71 443 :default

Although I see the returning packets getting denied:
Mar  5 20:23:13 vados kernel: ipfw: 9999 Deny TCP 141.135.72.71:443 109.140.18.212:10087 out via em0
Mar  5 20:23:16 vados kernel: ipfw: 9999 Deny TCP 141.135.72.71:443 109.140.18.212:10087 out via em0
Mar  5 20:23:19 vados kernel: ipfw: 9999 Deny TCP 141.135.72.71:443 109.140.18.212:10087 out via em0
Mar  5 20:23:22 vados kernel: ipfw: 9999 Deny TCP 141.135.72.71:443 109.140.18.212:10087 out via em0

Can somebody help me out with this? Did the behavior of limit source address change?
Comment 1 Dries Michiels 2019-03-05 19:35:52 UTC
Also, I just noticed that this is only a problem for rules matching on TCP.
Comment 2 Andrey V. Elsukov freebsd_committer 2019-03-06 08:07:02 UTC
(In reply to Dries Michiels from comment #0)
> After upgrading from source from r343710 to r344737 the behavior of
> limit-source address changed.

Hi, can you try revert r343931 and test your scenario?
You can do this with following command:
# cd /usr/src
# svn merge -c -343931 ^/stable/12
Comment 3 Andrey V. Elsukov freebsd_committer 2019-03-06 09:01:39 UTC
Created attachment 202644 [details]
Proposed patch (for stable/12)

Can you try this patch?
Comment 4 Dries Michiels 2019-03-06 10:31:53 UTC
Do I first revert to r343931 or can I apply the patch on r344737?

Thanks!
Comment 5 Andrey V. Elsukov freebsd_committer 2019-03-06 10:34:25 UTC
(In reply to Dries Michiels from comment #4)
> Do I first revert to r343931 or can I apply the patch on r344737?
> 
> Thanks!

You can just apply the patch, It reverts the part of r344737, where the bug is, I think.
Comment 6 Dries Michiels 2019-03-06 19:37:19 UTC
Yep, that fixes it! Thanks.
Comment 7 commit-hook freebsd_committer 2019-03-07 04:41:05 UTC
A commit references this bug:

Author: ae
Date: Thu Mar  7 04:40:45 UTC 2019
New revision: 344870
URL: https://svnweb.freebsd.org/changeset/base/344870

Log:
  Fix the problem with O_LIMIT states introduced in r344018.

  dyn_install_state() uses `rule` pointer when it creates state.
  For O_LIMIT states this pointer actually is not struct ip_fw,
  it is pointer to O_LIMIT_PARENT state, that keeps actual pointer
  to ip_fw parent rule. Thus we need to cache rule id and number
  before calling dyn_get_parent_state(), so we can use them later
  when the `rule` pointer is overrided.

  PR:		236292
  MFC after:	3 days

Changes:
  head/sys/netpfil/ipfw/ip_fw_dynamic.c
Comment 8 commit-hook freebsd_committer 2019-03-10 04:42:22 UTC
A commit references this bug:

Author: ae
Date: Sun Mar 10 04:41:31 UTC 2019
New revision: 344976
URL: https://svnweb.freebsd.org/changeset/base/344976

Log:
  MFC r344870:
    Fix the problem with O_LIMIT states introduced in r344018.

    dyn_install_state() uses `rule` pointer when it creates state.
    For O_LIMIT states this pointer actually is not struct ip_fw,
    it is pointer to O_LIMIT_PARENT state, that keeps actual pointer
    to ip_fw parent rule. Thus we need to cache rule id and number
    before calling dyn_get_parent_state(), so we can use them later
    when the `rule` pointer is overrided.

    PR:		236292

Changes:
_U  stable/12/
  stable/12/sys/netpfil/ipfw/ip_fw_dynamic.c
Comment 9 Andrey V. Elsukov freebsd_committer 2019-03-10 04:59:15 UTC
Fixed in head/ and stable/12. Thanks!
Comment 10 commit-hook freebsd_committer 2019-04-14 12:05:56 UTC
A commit references this bug:

Author: ae
Date: Sun Apr 14 12:05:09 UTC 2019
New revision: 346205
URL: https://svnweb.freebsd.org/changeset/base/346205

Log:
  MFC r341471:
    Reimplement how net.inet.ip.fw.dyn_keep_states works.

    Turning on of this feature allows to keep dynamic states when parent
    rule is deleted. But it works only when the default rule is
    "allow from any to any".

    Now when rule with dynamic opcode is going to be deleted, and
    net.inet.ip.fw.dyn_keep_states is enabled, existing states will reference
    named objects corresponding to this rule, and also reference the rule.
    And when ipfw_dyn_lookup_state() will find state for deleted parent rule,
    it will return the pointer to the deleted rule, that is still valid.
    This implementation doesn't support O_LIMIT_PARENT rules.

    The refcnt field was added to struct ip_fw to keep reference, also
    next pointer added to be able iterate rules and not damage the content
    when deleted rules are chained.

    Named objects are referenced only when states are going to be deleted to
    be able reuse kidx of named objects when new parent rules will be
    installed.

    ipfw_dyn_get_count() function was modified and now it also looks into
    dynamic states and constructs maps of existing named objects. This is
    needed to correctly export orphaned states into userland.

    ipfw_free_rule() was changed to be global, since now dynamic state can
    free rule, when it is expired and references counters becomes 1.

    External actions subsystem also modified, since external actions can be
    deregisterd and instances can be destroyed. In these cases deleted rules,
    that are referenced by orphaned states, must be modified to prevent access
    to freed memory. ipfw_dyn_reset_eaction(), ipfw_reset_eaction_instance()
    functions added for these purposes.

    Obtained from:	Yandex LLC
    Sponsored by:	Yandex LLC
    Differential Revision:	https://reviews.freebsd.org/D17532

  MFC r341472:
    Add ability to request listing and deleting only for dynamic states.

    This can be useful, when net.inet.ip.fw.dyn_keep_states is enabled, but
    after rules reloading some state must be deleted. Added new flag '-D'
    for such purpose.

    Retire '-e' flag, since there can not be expired states in the meaning
    that this flag historically had.

    Also add "verbose" mode for listing of dynamic states, it can be enabled
    with '-v' flag and adds additional information to states list. This can
    be useful for debugging.

    Obtained from:	Yandex LLC
    Sponsored by:	Yandex LLC

  MFC r344018:
    Remove `set' field from state structure and use set from parent rule.

    Initially it was introduced because parent rule pointer could be freed,
    and rule's information could become inaccessible. In r341471 this was
    changed. And now we don't need this information, and also it can become
    stale. E.g. rule can be moved from one set to another. This can lead
    to parent's set and state's set will not match. In this case it is
    possible that static rule will be freed, but dynamic state will not.
    This can happen when `ipfw delete set N` command is used to delete
    rules, that were moved to another set.
    To fix the problem we will use the set number from parent rule.

    Obtained from:	Yandex LLC
    Sponsored by:	Yandex LLC

  MFC r344870:
    Fix the problem with O_LIMIT states introduced in r344018.

    dyn_install_state() uses `rule` pointer when it creates state.
    For O_LIMIT states this pointer actually is not struct ip_fw,
    it is pointer to O_LIMIT_PARENT state, that keeps actual pointer
    to ip_fw parent rule. Thus we need to cache rule id and number
    before calling dyn_get_parent_state(), so we can use them later
    when the `rule` pointer is overrided.

    PR:		236292

Changes:
_U  stable/11/
  stable/11/sbin/ipfw/ipfw.8
  stable/11/sbin/ipfw/ipfw2.c
  stable/11/sbin/ipfw/ipfw2.h
  stable/11/sbin/ipfw/main.c
  stable/11/sys/netinet/ip_fw.h
  stable/11/sys/netpfil/ipfw/ip_fw_dynamic.c
  stable/11/sys/netpfil/ipfw/ip_fw_eaction.c
  stable/11/sys/netpfil/ipfw/ip_fw_private.h
  stable/11/sys/netpfil/ipfw/ip_fw_sockopt.c
  stable/11/sys/netpfil/ipfw/nat64/nat64lsn_control.c
  stable/11/sys/netpfil/ipfw/nat64/nat64stl_control.c
  stable/11/sys/netpfil/ipfw/nptv6/nptv6.c