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

sd-dhcp6-lease: Ignore invalid bytes at the end of the packet #28138

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

pkern
Copy link
Contributor

@pkern pkern commented Jun 23, 2023

Oracle Cloud sends malformed DHCPv6 replies that end in a DHCP option 0 containing a NoPrefixAvail status message (and thus should probably be an IA_PD) option and an additional null byte that does not parse as an option code.

networkd currently can cope with the invalid option (it is ignored), but the whole packet is ignored altogether because of the additional null at the end.

It's better to be liberal in what we accept and actually assign an address, given that the reply contains a valid IA_NA.

(Technically this is a glorified if 1 < 2 check. But it's checking for the condition that will yield -EBADMSG from dhcp6_option_parse.)

@github-actions github-actions bot added network please-review PR is ready for (re-)review by a maintainer labels Jun 23, 2023
@keszybz
Copy link
Member

keszybz commented Jun 23, 2023

Looks reasonble, but I wonder how liberal we should be:

  1. only ignore the trailing bytes if they are 0
  2. ignore anything that is shorter than DHCP6Option header
  3. ignore all errors from dhcp6_option_parse

The patch does 2., but I'm not sure what the most correct thing to do is. @yuwata comments?

@pkern
Copy link
Contributor Author

pkern commented Jun 23, 2023

The options are TLV so for 3) I'd fear that we lose synchronization. At the end of the packet this works. The unknown option is also ignored as intended.

Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, if it works.
Could you provide the dumped packet (feel free to hide privacy data if you'd like) and add a test based on the packet?

@yuwata yuwata added the dhcp label Jun 25, 2023
@pkern
Copy link
Contributor Author

pkern commented Jun 27, 2023

Is there an existing test harness to put this in? I only saw an integration test that runs a DHCPv6 server.

The packet looks like this:

0000   02 00 17 07 d8 be 00 00 17 4e 8b fb 86 dd 60 00
0010   00 00 00 82 11 40 fe 80 00 00 00 00 00 00 02 00
0020   17 ff fe 4e 8b fb fe 80 00 00 00 00 00 00 00 00
0030   17 ff fe 07 d8 be 02 23 02 22 00 82 91 1c 07 01
0040   9f d6 00 02 00 0a 00 03 00 01 00 00 17 4e 8b fb
0050   00 01 00 0e 00 02 00 00 ab 11 23 bb 0b 9a 28 b8
0060   29 b8 00 0e 00 00 00 03 00 28 80 c0 db 71 00 00
0070   a8 c0 00 00 fd 20 00 05 00 18 26 03 c0 20 80 16
0080   44 00 6b 3b 60 bd 17 a0 40 c7 00 01 51 80 00 01
0090   5f 90 00 00 00 21 00 00 00 00 00 00 00 00 00 00
00a0   00 00 00 0d 00 11 00 06 4e 6f 50 72 65 66 69 78
00b0   41 76 61 69 6c 61 63 00

@yuwata
Copy link
Member

yuwata commented Jun 29, 2023

Please update src/libsystemd-network/test-dhcp6-client.c, e.g. copy TEST(client_parse_message_issue_24002) and modify the packet binary based on what you paste.

@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jun 29, 2023
@yuwata yuwata linked an issue Jun 29, 2023 that may be closed by this pull request
@github-actions github-actions bot added tests please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jun 29, 2023
pkern and others added 2 commits June 30, 2023 01:21
Oracle Cloud sends malformed DHCPv6 replies that have an invalid
byte at the end, which cannot be parsed as an option code.

networkd currently can cope with the invalid option (it is ignored),
but the whole packet is ignored altogether because of the additional
null at the end.

It's better to be liberal in what we accept and actually assign an
address, given that the reply contains a valid IA_NA.

Fixes systemd#28183.
@yuwata
Copy link
Member

yuwata commented Jun 29, 2023

Added a brief test about that, and worked fine for me. Setting the green label.

@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed needs-stable-backport and removed please-review PR is ready for (re-)review by a maintainer labels Jun 29, 2023
@bluca bluca merged commit 1d2b93f into systemd:main Jun 29, 2023
44 of 48 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jun 29, 2023
@pkern
Copy link
Contributor Author

pkern commented Jun 30, 2023

Thank you, @yuwata and @bluca!

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

Successfully merging this pull request may close these issues.

DHCPv6 client ignores packets with invalid bytes at the end
4 participants