iptables: segfault when renaming a chain

[Louis Bouchard]

[ Impact ]
 * An explanation of the effects of the bug on users

This is the description for the upstream fix of this bug[1] :

This is an odd bug: If the number of chains is right and one renames the
last one in the list, libiptc dereferences a NULL pointer.

 * justification for backporting the fix to the stable release.
Without this patch, users may experience segmentation fault when using
the following versions of iptables :

  - Bionic : iptables
  - Focal : iptables
  - Jammy : iptables-legacy
  - Kinetic: iptables-legacy

 * In addition, it is helpful, but not required, to include an
   explanation of how the upload fixes this bug.

The upstream fix adjust the size of the chain_index if the element is the
last chain in the list.

[1] http://git.netfilter.org/iptables/commit/?id=97bf4e68fc0794adba3243fd96f40f4568e7216f

[ Test Plan ]

 * detailed instructions how to reproduce the bug

 The following code (adapted from the upstream commit to work on Kinetic) may be used to reproduce the issue :
----------------------------------------8<--------------------------------
#!/bin/bash
#
# Cover for a bug in libiptc:
# - the chain 'node-98-tmp' is the last in the list sorted by name
# - there are 81 chains in total, so three chain index buckets
# - the last index bucket contains only the 'node-98-tmp' chain
# => rename temporarily removes it from the bucket, leaving a NULL bucket
# behind which is dereferenced later when inserting the chain again with new
# name again

(
 echo "*filter"
 for chain in node-1 node-10 node-101 node-102 node-104 node-107 node-11 node-12 node-13 node-14 node-15 node-16 node-17 node-18 node-19 node-2 node-20 node-21 node-22 node-23 node-25 node-26 node-27 node-28 node-29 node-3 node-30 node-31 node-32 node-33 node-34 node-36 node-37 node-39 node-4 node-40 node-41 node-42 node-43 node-44 node-45 node-46 node-47 node-48 node-49 node-5 node-50 node-51 node-53 node-54 node-55 node-56 node-57 node-58 node-59 node-6 node-60 node-61 node-62 node-63 node-64 node-65 node-66 node-68 node-69 node-7 node-70 node-71 node-74 node-75 node-76 node-8 node-80 node-81 node-86 node-89 node-9 node-92 node-93 node-95 node-98-tmp; do
  echo ":$chain - [0:0]"
 done
 echo "COMMIT"
) | $XT_MULTI iptables-legacy-restore
$XT_MULTI iptables-legacy -E node-98-tmp node-98
exit $?
---------------------------------------->8--------------------------------

Alternatively, this test has been added to the DEP8 list of tests, and will be executed automatically once the package is accepted into proposed. The DEP8 logs can be inspected for its run. Look for a test named "0006rename-segfault".

[ Where problems could occur ]

For Jammy and onward, only users of the -legacy commands may be affected.
Since Jammy, iptables uses the new nft libraries which are not affected
by the bug.

For Bionic and Focal users, the regular iptables command is affected by
the change.

As stated in the manpage :
E, --rename-chain old-chain new-chain
              Rename the user specified chain to the user supplied name. This is cosmetic, and has no effect on the structure of the table.

In case of a problem, only the modification of the name would be affected
as this is clearly outlined as a cosmetic only change.

[ Other Info ]
The patch is also applied to lunar and mantic, but is fixed in upstream's 1.8.9 release which so far is only in debian testing/unstable.

This is being uploaded together with test fixes from bug #1992454 (bionic-specific) and bug #2019023 (focal-specific), which were found and fixed while trying out the DEP8 runs for this package.

[Robie Basak (racb)]

Thank you for contributing this fix!

This looks fine; just one comment. Upstream adds a test. But it looks to me like it won't run because the quilt patch can't handle the executable bit. Looks like there's a workaround in debian/tests/control for another case of the same issue. Please could you ensure that dep8 tests are passing (in "isolation-machine") and running this new test?

[Louis Bouchard (louis)]

Hello,

Thanks for picking this up. Reference to the test was added to debian/test/control & test ran fine as we can see in the logs :

Removing autopkgtest-satdep (0) ...
autopkgtest [17:00:57]: test command18: chmod +x ./iptables/tests/shell/testcases/chain/0006rename-segfault_0; cd iptables/tests/shell; ./run-tests.sh --host
autopkgtest [17:00:57]: test command18: [-----------------------

I: [OK] ././testcases/arptables/0001-arptables-save-restore_0
I: [OK] ././testcases/arptables/0002-arptables-restore-defaults_0
I: [OK] ././testcases/arptables/0003-arptables-verbose-output_0
I: [OK] ././testcases/chain/0001duplicate_1
I: [OK] ././testcases/chain/0002newchain_0
I: [OK] ././testcases/chain/0003rename_1
I: [OK] ././testcases/chain/0006rename-segfault_0

Here is a new debdiff with the added bits.

[Louis Bouchard (louis)]

Slightly modified debdiff which will only run the tests once.

[Louis Bouchard (louis)]

Here is an autopkgtest run with the latest patch :

Removing autopkgtest-satdep (0) ...
autopkgtest [14:09:03]: test command17: chmod +x ./iptables/tests/shell/testcases/iptables/0007-zero-counters_0 ./iptables/tests/shell/testcases/chain/0006rename-segfault_0; cd iptables/tests/shell; ./run-testst
autopkgtest [14:09:03]: test command17: [-----------------------

I: [OK] ././testcases/arptables/0001-arptables-save-restore_0
I: [OK] ././testcases/arptables/0002-arptables-restore-defaults_0
I: [OK] ././testcases/arptables/0003-arptables-verbose-output_0
I: [OK] ././testcases/chain/0001duplicate_1
I: [OK] ././testcases/chain/0002newchain_0
I: [OK] ././testcases/chain/0003rename_1
I: [OK] ././testcases/chain/0006rename-segfault_0
...
autopkgtest [14:09:19]: test command17: - - - - - - - - - - results - - - - - - - - - -
command17 PASS
autopkgtest [14:09:19]: @@@@@@@@@@@@@@@@@@@@ summary
command1 PASS
command2 PASS
command3 PASS
command4 PASS
command5 PASS
command6 PASS
command7 PASS
command8 PASS
command9 PASS
command10 PASS
command11 PASS
command12 PASS
command13 PASS
command14 PASS
command15 PASS
command16 PASS
command17 PASS

[Andreas Hasenack (ahasenack)]

Hi Louis, Robie

I see that Robie sponsored the kinetic upload. Will the other releases follow suit?

Louis, would you mind updating the bug description with the SRU template[1] filled in? I see the test case, so the remaining steps would be to fill in the [Impact] section, and, importantly, the [Where things could go wrong] one.

1. https://wiki.ubuntu.com/StableReleaseUpdates#SRU_Bug_Template

[Louis Bouchard (louis)]

Hello,
I was waiting for the kinetic upload to get through and I have the debdiffs ready for the SRU.

I'll take care of that & the SRU template in the coming days.
...Louis

[Andreas Hasenack (ahasenack)]

I think we will need newer version numbers.

 iptables | 1.8.7-1ubuntu6 | kinetic | source
 iptables | 1.8.7-1ubuntu6 | lunar | source

And kinetic has 1ubuntu7 in unapproved, because it was uploaded while kinetic was the development release.

We will need 1ubuntu7 in lunar, and 1ubuntu6.1 in kinetic. Whoever sponsors these can take care of this small adjustment version anyway, just a heads up. Lunar is still in a bit of a freeze at the moment anyway.

[Louis Bouchard (louis)]

Here are the debdiffs for Bionic, Focal and Jammy. I will let the sponsor worry about the numbering for Kinetic & Lunar

[Louis Bouchard (louis)]

Debdiff for Bionic

[Louis Bouchard (louis)]

Debdiff for Focal

[Louis Bouchard (louis)]

Debdiff for Jammy

[Andreas Hasenack (ahasenack)]

Thanks, I'll take care of it

[Andreas Hasenack (ahasenack)]

I'm going to reject the upload in the kinetic unapproved queue, since it was superseded by lunar, adjust the versions and sponsor to lunar and the rest.

[Andreas Hasenack (ahasenack)]

In jammy you changed the test depends in d/t/control, any particular reason?
-Depends: @, nftables:native
+Depends: @

In focal, this particular test didn't exist before, and you are adding it (thanks!), and with the "nftables:native" depends.

[Louis Bouchard (louis)]

Hello,

Sorry for the late response. It is most probably a mistake from my part. Let me know if you want me to upload a corrected debdiff.

Regarding focal, since it was added by upstream, I thought that it would be better to add it as well.

Kind regards,
...Louis

[Launchpad Janitor (janitor)]

This bug was fixed in the package iptables - 1.8.7-1ubuntu7

---------------
iptables (1.8.7-1ubuntu7) lunar; urgency=medium

  * d/patches: 0001-libiptc-Fix-for-segfault-when-renaming-a-chain.patch
    Apply upstream fix :
    - libiptc: Fix for segfault when renaming a chain (LP: #1992454)

 -- Louis Bouchard <email address hidden> Tue, 11 Oct 2022 14:26:53 +0200

[Andreas Hasenack (ahasenack)]

This finally migrated in lunar, so I can sponsor/adjust the other uploads. Will try to get to this tomorrow.

[Louis Bouchard (louis)]

Great ! I appreciate your help on this.
...Louis

[Louis Bouchard (louis)]

Hello,

Is there any chance of seeing this fix being SRUed to the stable releases ?

Kind regards,

...Louis

[Andreas Hasenack (ahasenack)]

You are too kind, Louis. I'm sorry I dropped this again. I'm tagging it now, so it appears in our team's weekly checkpoint, and will address it.

[Andreas Hasenack (ahasenack)]

Adjusting the DEP8 tests in all releases.

Bionic doesn't even have the test runner, so I just added the new segfault test as an individual script call.

Focal has the test runner, but many tests fail, so I'm using the runner to run just this new test (as per your debdiff).

Jammy and Kinetic can run all tests it seems.

[Andreas Hasenack (ahasenack)]

I fixed the failing shell test in the case of focal (see bug #2019023) and was able to run all shell tests there, and for bionic I fixed the other failing DEP8 tests so we can get a clean run there too (see bug #2019022).

[Andreas Hasenack (ahasenack)]

Uploaded b, f, j, k. They are in the unapproved queue now.

[Louis Bouchard (louis)]

Hello, thanks for the update. Will test when they show up in proposed.

[Timo Aaltonen (tjaalton)]

Hello Louis, or anyone else affected,

Accepted iptables into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/iptables/1.8.7-1ubuntu5.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Timo Aaltonen (tjaalton)]

Hello Louis, or anyone else affected,

Accepted iptables into kinetic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/iptables/1.8.7-1ubuntu6.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-kinetic to verification-done-kinetic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-kinetic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Timo Aaltonen (tjaalton)]

Hello Louis, or anyone else affected,

Accepted iptables into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/iptables/1.8.4-3ubuntu2.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Timo Aaltonen (tjaalton)]

Hello Louis, or anyone else affected,

Accepted iptables into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/iptables/1.6.1-2ubuntu2.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Ubuntu SRU Bot (ubuntu-sru-bot)]

All autopkgtests for the newly accepted iptables (1.8.7-1ubuntu6.1) for kinetic have finished running.
The following regressions have been reported in tests triggered by the package:

nova/3:26.1.0-0ubuntu2.1 (armhf)
west-chamber/20100405+svn20111107.r124-14 (i386)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/kinetic/update_excuses.html#iptables

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

[Andreas Hasenack (ahasenack)]

I cleared the DEP8 failures:
- nova: retry fixed it
- west-chamber/i386: migration-reference/0 is also failing, so that cleared it

[Ubuntu SRU Bot (ubuntu-sru-bot)]

All autopkgtests for the newly accepted iptables (1.8.4-3ubuntu2.1) for focal have finished running.
The following regressions have been reported in tests triggered by the package:

nova/2:21.2.4-0ubuntu2.3 (armhf)
systemd/245.4-4ubuntu3.21 (armhf, ppc64el, s390x)
ufw/0.36-6ubuntu1 (amd64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/focal/update_excuses.html#iptables

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

[Ubuntu SRU Bot (ubuntu-sru-bot)]

All autopkgtests for the newly accepted iptables (1.8.7-1ubuntu5.1) for jammy have finished running.
The following regressions have been reported in tests triggered by the package:

nova/3:25.1.0-0ubuntu2.1 (armhf)
systemd/249.11-0ubuntu3.9 (amd64, arm64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/jammy/update_excuses.html#iptables

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

[Andreas Hasenack (ahasenack)]

These new DEP8 failures are now also cleared.

[Louis Bouchard (louis)]

Hello,
Thank you for your help in uploading & fixing the DEP8 tests.

I have tested the packages in -proposed using the test script in the SRU description for :

- bionic
- focal
- jammy
- kinetic.

I confirm that the new package fix the problem in those distros.

Thanks.

[Robie Basak (racb)]

Normally testing that the bug is fixed would verify that the package still works, but it isn't clear to me that this is the case for this (very narrow) test plan.

Has there been any testing done that verifies that iptables still basically works, please? I looked at the build logs (for Bionic as an example) and the dep8 tests, and I don't see any general test suite being run.

[Louis Bouchard (louis)]

Hi Robie,

This version of the package has been in Lunar since end of last year so I assume that it has been largely in use since then.

..Louis

[Andreas Hasenack (ahasenack)]

iptables in excuses triggers tests in other packages. The excuses report doesn't show the fully green results anymore, so it's easy to miss them.

I think we can consider the ufw dep8 tests as sufficient for the case of "normal iptables usage hasn't regressed".

I ran the ufw debian/tests/root-unittest DEP8 test in a jammy vm, and checked with execsnoop that it was calling the real iptables during the tests, and not just pretending or calling a fake binary like the normal unittest test. I aborted it after a few minutes, because the extra logging was taking a lot of time, but here is a sample:

17:12:30 TIME TIME(s) UID PCOMM PID PPID RET ARGS
17:12:30 1.962 0 iptables 28768 28767 0 /usr/sbin/iptables --version
17:12:30 1.964 0 iptables 28773 28772 0 /usr/sbin/iptables --version
17:12:30 2.002 0 iptables 28817 28816 0 /usr/sbin/iptables -V
17:12:30 2.203 0 iptables 29060 29059 0 /usr/sbin/iptables -V
17:12:30 2.205 0 ip6tables 29062 29061 0 /sbin/ip6tables -L INPUT -n
17:12:30 2.205 0 iptables 29063 29061 0 /sbin/iptables -F ufw-logging-deny
17:12:30 2.206 0 iptables 29064 29061 0 /sbin/iptables -F ufw-logging-allow
(...)
17:12:30 2.552 0 iptables 29371 29225 0 /usr/sbin/iptables -D ufw-user-logging-forward -j RETURN
17:12:30 2.553 0 iptables 29372 29225 0 /usr/sbin/iptables -A ufw-after-logging-input -j LOG --log-prefix [UFW BLOCK] -m limit --limit 3/min --limit-burst 10
17:12:30 2.553 0 iptables 29373 29225 0 /usr/sbin/iptables -A ufw-after-logging-forward -j LOG --log-prefix [UFW BLOCK] -m limit --limit 3/min --limit-burst 10
17:12:30 2.554 0 iptables 29374 29225 0 /usr/sbin/iptables -I ufw-logging-deny -m conntrack --ctstate INVALID -j RETURN -m limit --limit 3/min --limit-burst 10
17:12:30 2.555 0 iptables 29375 29225 0 /usr/sbin/iptables -A ufw-logging-deny -j LOG --log-prefix [UFW BLOCK] -m limit --limit 3/min --limit-burst 10
17:12:30 2.555 0 iptables 29376 29225 0 /usr/sbin/iptables -A ufw-logging-allow -j LOG --log-prefix [UFW ALLOW] -m limit --limit 3/min --limit-burst 10
17:12:30 2.556 0 iptables 29377 29225 0 /usr/sbin/iptables -D ufw-user-limit -m limit --limit 3/minute -j LOG --log-prefix [UFW LIMIT BLOCK]
17:12:30 2.557 0 iptables 29378 29225 0 /usr/sbin/iptables -I ufw-user-limit -m limit --limit 3/minute -j LOG --log-prefix [UFW LIMIT BLOCK]
17:12:31 2.601 0 iptables 29380 29379 0 /usr/sbin/iptables -V
17:12:31 2.609 0 iptables 29383 29057 0 /usr/sbin/iptables -L -n
(...)

# grep iptables d-t-root-unittest.log |wc -l
9389

All these while iptables from jammy-proposed was installed:
# apt-cache policy iptables
iptables:
  Installed: 1.8.7-1ubuntu5.1
  Candidate: 1.8.7-1ubuntu5.1
  Version table:
 *** 1.8.7-1ubuntu5.1 500
        500 http://br.archive.ubuntu.com/ubuntu jammy-proposed/main amd64 Packages

With that in mind, let's confirm that the ufw dep8 tests ran with the iptables package from propose...

[Louis Bouchard (louis)]

Hello,

As noted in the bug description, it is important to know that from Jammy onward, the only way to exercise the bugfix is to use iptables-legacy and not iptables which uses a newer library.

It also mitigates the impact on newer releases unless the -legacy version is being used.

...Louis

[Launchpad Janitor (janitor)]

This bug was fixed in the package iptables - 1.6.1-2ubuntu2.1

---------------
iptables (1.6.1-2ubuntu2.1) bionic; urgency=medium

  [ Louis Bouchard ]

  * d/patches: 0001-libiptc-Fix-for-segfault-when-renaming-a-chain.patch
    Apply upstream fix:
    - libiptc: Fix for segfault when renaming a chain (LP: #1992454)
    - include new test for this issue in the DEP8 run

  [ Andreas Hasenack ]
  * d/t/control: fix DEP8 tests (LP: #2019022)

 -- Andreas Hasenack <email address hidden> Tue, 09 May 2023 15:42:18 -0300

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for iptables has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package iptables - 1.8.4-3ubuntu2.1

---------------
iptables (1.8.4-3ubuntu2.1) focal; urgency=medium

  [ Louis Bouchard ]
  * d/p/0001-libiptc-Fix-for-segfault-when-renaming-a-chain.patch: apply
    upstream fix for segfault when renaming a chain. Includes a test
    case (LP: #1992454)

  [ Andreas Hasenack ]
  * Fix shell tests, and run them as DEP8 (LP: #2019023):
    - d/p/0501-fix-firewalld-shell-test.patch: fix the globbing expression
      which was used to decide whether to sort the restore output or not
      before comparing it to the good case
    - d/t/control: run all shell tests as DEP8

 -- Andreas Hasenack <email address hidden> Tue, 09 May 2023 15:39:57 -0300

[Launchpad Janitor (janitor)]

This bug was fixed in the package iptables - 1.8.7-1ubuntu6.1

---------------
iptables (1.8.7-1ubuntu6.1) kinetic; urgency=medium

  * d/patches: 0001-libiptc-Fix-for-segfault-when-renaming-a-chain.patch
    Apply upstream fix:
    - libiptc: Fix for segfault when renaming a chain (LP: #1992454)
    - include new test for this issue in the DEP8 run

 -- Louis Bouchard <email address hidden> Wed, 26 Apr 2023 11:45:27 -0300

[Launchpad Janitor (janitor)]

This bug was fixed in the package iptables - 1.8.7-1ubuntu5.1

---------------
iptables (1.8.7-1ubuntu5.1) jammy; urgency=medium

  * d/patches: 0001-libiptc-Fix-for-segfault-when-renaming-a-chain.patch
    Apply upstream fix:
    - libiptc: Fix for segfault when renaming a chain (LP: #1992454)
    - include new test for this issue in the DEP8 run

 -- Louis Bouchard <email address hidden> Wed, 26 Apr 2023 11:40:37 -0300