Skip to content

resolved: actually check authenticated flag of SOA transaction#30549

Merged
yuwata merged 1 commit intosystemd:mainfrom
msekletar:dnssec-unsigned-a
Dec 21, 2023
Merged

resolved: actually check authenticated flag of SOA transaction#30549
yuwata merged 1 commit intosystemd:mainfrom
msekletar:dnssec-unsigned-a

Conversation

@msekletar
Copy link
Copy Markdown
Contributor

@msekletar msekletar commented Dec 20, 2023

Fixes #25676

@github-actions github-actions bot added resolve please-review PR is ready for (re-)review by a maintainer labels Dec 20, 2023
@msekletar
Copy link
Copy Markdown
Contributor Author

msekletar commented Dec 20, 2023

I tried to take a stab at #25676 and this is what I came up with, it seems to fix the issue but please review it carefully as I am not really resolved expert.

yuwata
yuwata requested changes Dec 20, 2023
View reviewed changes
@yuwata yuwata added needs-stable-backport 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 Dec 20, 2023
@github-actions github-actions bot added 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 Dec 20, 2023
@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 and removed please-review PR is ready for (re-)review by a maintainer labels Dec 20, 2023
@yuwata
Copy link
Copy Markdown
Member

yuwata commented Dec 20, 2023

@poettering Could you double check?

@yuwata yuwata requested a review from poettering December 20, 2023 18:33
@poettering
Copy link
Copy Markdown
Member

poettering commented Dec 21, 2023

ouch.

lgtm

@yuwata yuwata merged commit 3b4cc14 into systemd:main Dec 21, 2023
@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 Dec 21, 2023
@AgentOak AgentOak mentioned this pull request Dec 22, 2023
Closed
@YHNdnzj YHNdnzj linked an issue Dec 22, 2023 that may be closed by this pull request
DNSSEC doesn't prevent MITM #15158
Closed
@pemensik
Copy link
Copy Markdown
Contributor

pemensik commented Dec 22, 2023 •
edited
Loading

I have taken a look into this commit and it seemed a way too simple to fix the issue properly. I made my own rawhide build with added only this patch on top of last build.

As I have suspected this change IS NOT sufficient to prevent the cache poisoning. I have used simple addition to libvirt's dnsmasq backed network.

# sudo virsh net-edit default
<network>
  <!-- ... -->
  <dns>
    <txt name='example.net' value='v=spf1 +all'/>
    <host ip='127.0.0.255'>
      <hostname>example.net</hostname>
    </host>
  </dns>
  <!-- ... -->
</network>

Unless I have made mistake with build, this is not sufficient fix. It may protect SOA query itself, but does not protect other records. It is still possible to spoof own records. All example domains ARE signed, so any their spoofing should make them broken and inaccessible.

# resolvectl dnssec
Global: yes
Link 2 (host0): yes
# rpm -q systemd-resolved
systemd-resolved-255.1-2.fc40.x86_64
# resolvectl query example.net
example.net: 127.0.0.255                       -- link: host0
             2606:2800:220:1:248:1893:25c8:1946 -- link: host0

-- Information acquired via protocol DNS in 5.9ms.
-- Data is authenticated: no; Data was acquired via local or encrypted transport: no
-- Data from: cache network
# resolvectl query -t txt example.net
example.net IN TXT "v=spf1 +all"                            -- link: host0

-- Information acquired via protocol DNS in 2.1ms.
-- Data is authenticated: no; Data was acquired via local or encrypted transport: no
-- Data from: network
# delv example.net
;; resolution failed: SERVFAIL
# unbound-host -rvD example.net
example.net has address 127.0.0.255 (BOGUS (security failure))
validation failure <example.net. A IN>: no signatures from 127.0.0.53
example.net has IPv6 address 2606:2800:220:1:248:1893:25c8:1946 (secure)
example.net mail is handled by 0 . (secure)

Edit: Fixed delv and unbound-host checks with LLMNR=no.

@pemensik pemensik mentioned this pull request Dec 22, 2023
Closed
pemensik
pemensik reviewed Dec 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@pemensik pemensik left a comment

Choose a reason for hiding this comment

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

I am afraid it is not as simple as demanding AD bit in SOA queries. First problem is those SOA queries are not necessary anyway. But it is possible to keep SOA query intact and replace just record type you want to modify. That is done by dnsmasq for example, it does not care about zone definitions.

@pemensik
Copy link
Copy Markdown
Contributor

pemensik commented Dec 22, 2023

False alarm, this seems to fix the issue well, but Fedora package does not contain instruction to restart systemd-resolved.service, therefore it had no immediate effect.

@jamacku jamacku mentioned this pull request Jan 3, 2024
Merged
@keszybz
Copy link
Copy Markdown
Member

keszybz commented Jan 23, 2024

@pemensik Thanks for testing. In case anyone looks here, both issues should now be fixed in Fedora: the patch is included in backports and package scriptlets have been update to restart systemd-resolved on package upgrades.

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

Reviewers

@yuwata yuwata yuwata approved these changes

@poettering poettering Awaiting requested review from poettering

+1 more reviewer

@pemensik pemensik pemensik left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

resolve

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

resolved DNSSEC validation can be bypassed by MITM DNSSEC doesn't prevent MITM

5 participants

@msekletar @yuwata @poettering @pemensik @keszybz