ACK: [SRU T][PATCH 0/3] netfilter: nf_conncount: fix for LP#1811094

[Stefan Bader]

On 14.01.19 19:55, Mauricio Faria de Oliveira wrote:
> BugLink: https://bugs.launchpad.net/bugs/1811094
> 
> [Notes for Trusty]
> 
> The backport for Trusty differs in some ways than for Xenial/Bionic.
> The delta is large enough that deps for a "cleaner" backport become
> a lot, and intrusive.  That cleaning in Xenial/Bionic happened into
> functions that don't yet exist in Trusty, so aren't needed here.
> 
> Patch 1 helps with that a bit, but it's actually needed to move the
> non-NULL checks down, so that IS_ERR() can be used before those.
> 
> Patch 2 is the fix per se, and has the most changes, all described
> in the backport/provenance section, but those seem straightforward.
> 
> Patch 3 is a fix for that, trivial change.
> 
> [Impact]
> 
>  * The iptables connection count/limit rules can be breached
>    with multithreaded network driver/server/client (common)
>    due to a race in the conncount/connlimit code.
> 
>  * For example:
> 
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
> 
>  * The fix is a backport from an upstream commit that resolves
>    the problem that address the race condition (plus one fix).
> 
>    commit b36e4523d4d5 ("netfilter: nf_conncount: fix garbage
>    collection confirm race").
> 
> [Test Case]
> 
>  * Server-side: (relevant kernel side)
>    (limit TCP port 7777 to only 2000 connections)
> 
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
> 
>    # ulimit -SHn 65000 # increase number of open files
>    # ruby server.rb # multi-threaded server
> 
>  * Client-side:
> 
>    # ulimit -SHn 65000
>    # ruby client.rb <server ip> <port> <target # connections> <# threads>
>    <test output>
> 
>  * Results with Original kernel:
>    (client achieves target of 6000 connections > limit of 2000 connections)
> 
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    6000
>    Target reached. Thread finishing
>    6001
>    Target reached. Thread finishing
>    6002
>    Target reached. Thread finishing
>    Threads done. 6002 connections
>    press enter to exit
> 
>  * Results with Modified kernel:
>    (client is limited to 2000 connections, and times out afterward)
> 
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    2000
>    <... blocks for a few minutes ...>
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    Threads done. 2000 connections
>    press enter to exit
> 
>  * Test cases possibly available upon request,
>    depending on original author's permission.
> 
> [Regression Potential]
> 
>  * The changes are limited to netfilter connlimit/conncount (names
>    change between older/newer kernel versions).
> 
> Florian Westphal (3):
>   netfilter: connlimit: improve packet-to-closed-connection logic
>   netfilter: nf_conncount: fix garbage collection confirm race
>   netfilter: nf_conncount: don't skip eviction when age is negative
> 
>  net/netfilter/xt_connlimit.c | 62 +++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> 

Somewhat not simple to review but I think the spirit of the changes look ok. And
it changes a very specific function which can be verified.
Wondering whether the security might be interested in that code to add it to the
QRT suite.

Acked-by: Stefan Bader <stefan.bader at canonical.com>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20190115/0c2e1bd7/attachment.sig>