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

Prevent possible downgrade attacks in the recursor #5889

Merged
merged 3 commits into from Nov 6, 2017

Conversation

Projects
None yet
2 participants
@pieterlexis
Member

pieterlexis commented Nov 1, 2017

Short description

RFC 4509 section 3: "Validator implementations SHOULD ignore DS RR
containing SHA-1 digests if DS RRs with SHA-256 digests are present in the
DS RRset."

As SHA348 is specified as well, the spirit of the this line is "use the
best algorithm".

This also means that if a delegation has DS records for multiple keys
(and algos) and only a subset have stronger digests, we will discard the
DS records for the weaker digests.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@pieterlexis pieterlexis requested a review from rgacogne Nov 1, 2017

@@ -1513,12 +1513,21 @@ vState SyncRes::getDSRecords(const DNSName& zone, dsmap_t& ds, bool taOnly, unsi
if (rcode == RCode::NoError || (rcode == RCode::NXDomain && !bogusOnNXD)) {
uint8_t bestDigestType = 0;

This comment has been minimized.

@rgacogne

rgacogne Nov 1, 2017

Member

It might be cleaner to use a DNSSECKeeper::dsdigestalgorithm_t instead.

@rgacogne

rgacogne Nov 1, 2017

Member

It might be cleaner to use a DNSSECKeeper::dsdigestalgorithm_t instead.

This comment has been minimized.

@pieterlexis

pieterlexis Nov 1, 2017

Member

that does mean also adding #include "dnsseckeeper.hh" in the syncres

@pieterlexis

pieterlexis Nov 1, 2017

Member

that does mean also adding #include "dnsseckeeper.hh" in the syncres

This comment has been minimized.

@pieterlexis

pieterlexis Nov 1, 2017

Member

DSRecordContent.d_digesttype is a uint8_t, so this means that we'll need to convert between uint8_t and DNSSECKeeper::dsdigestalgorithm_t every time. I've kept this a uint8_t but will use the enum for comparison.

@pieterlexis

pieterlexis Nov 1, 2017

Member

DSRecordContent.d_digesttype is a uint8_t, so this means that we'll need to convert between uint8_t and DNSSECKeeper::dsdigestalgorithm_t every time. I've kept this a uint8_t but will use the enum for comparison.

Show outdated Hide outdated pdns/syncres.cc Outdated
Show outdated Hide outdated pdns/syncres.cc Outdated
Show outdated Hide outdated pdns/recursordist/test-syncres_cc.cc Outdated
Show outdated Hide outdated pdns/recursordist/test-syncres_cc.cc Outdated
Show outdated Hide outdated pdns/recursordist/test-syncres_cc.cc Outdated
Show outdated Hide outdated pdns/recursordist/test-syncres_cc.cc Outdated
Show outdated Hide outdated pdns/recursordist/test-syncres_cc.cc Outdated
Show outdated Hide outdated pdns/recursordist/test-syncres_cc.cc Outdated
Show outdated Hide outdated pdns/recursordist/test-syncres_cc.cc Outdated

@rgacogne rgacogne added this to the rec-4.1.0 milestone Nov 1, 2017

@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis

pieterlexis Nov 1, 2017

Member

Travis failures are for the auth and look unrelated to this change:

$ git status
HEAD detached at FETCH_HEAD
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   modules/remotebackend/.bundle/config

no changes added to commit (use "git add" and/or "git commit -a")
$ git status | grep -q clean

The command "git status | grep -q clean" failed and exited with 1 during .

Your build has been stopped.
Member

pieterlexis commented Nov 1, 2017

Travis failures are for the auth and look unrelated to this change:

$ git status
HEAD detached at FETCH_HEAD
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   modules/remotebackend/.bundle/config

no changes added to commit (use "git add" and/or "git commit -a")
$ git status | grep -q clean

The command "git status | grep -q clean" failed and exited with 1 during .

Your build has been stopped.
@rgacogne

This comment has been minimized.

Show comment
Hide comment
@rgacogne

rgacogne Nov 1, 2017

Member

We don't apply this new logic to TAs, by the way, but it's probably fine.

Member

rgacogne commented Nov 1, 2017

We don't apply this new logic to TAs, by the way, but it's probably fine.

@rgacogne

Nice work!

pieterlexis added some commits Oct 31, 2017

recursor: Prevent DNSSEC downgrade attacks
RFC 4509 section 3: "Validator implementations SHOULD ignore DS RR
containing SHA-1 digests if DS RRs with SHA-256 digests are present in the
DS RRset."

As SHA348 is specified as well, the spirit of the this line is "use the
best algorithm".

This also means that if a delegation has DS records for multiple keys
(and algos) and only a subset have stronger digests, we will discard the
DS records for the weaker digests.
Add tests for DS downgrade protection
Adds an ugly hack to be able to test private functions in the syncres as
well.
@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis

pieterlexis Nov 2, 2017

Member

Rebased so travis can become green

Member

pieterlexis commented Nov 2, 2017

Rebased so travis can become green

@pieterlexis pieterlexis merged commit cb3cc53 into PowerDNS:master Nov 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pieterlexis pieterlexis deleted the pieterlexis:rec-41-prevent-downgrade branch Nov 6, 2017

@pieterlexis pieterlexis restored the pieterlexis:rec-41-prevent-downgrade branch Nov 7, 2017

@pieterlexis pieterlexis deleted the pieterlexis:rec-41-prevent-downgrade branch Dec 5, 2017

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