Skip to content

Commit

Permalink
policy docs: warn about filters and forwarding
Browse files Browse the repository at this point in the history
We've been notified about possibility of "cache poisoning" this way,
so let's document this drawback to make the expectations clearer.
  • Loading branch information
vcunat committed Dec 22, 2021
1 parent e862a78 commit ccb9d97
Showing 1 changed file with 14 additions and 0 deletions.
14 changes: 14 additions & 0 deletions modules/policy/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,20 @@ Actions :func:`policy.FORWARD`, :func:`policy.TLS_FORWARD` and :func:`policy.STU
`0x20 randomization <https://tools.ietf.org/html/draft-vixie-dnsext-dns0x20-00>`_.
See example in `Replacing part of the DNS tree`_.

.. warning::
Limiting forwarding actions by filters (e.g. :func:`policy.suffix`) may have unexpected consequences.
Notably, forwarders can inject *any* records into your cache
even if you "restrict" them to an insignificant DNS subtree --
except in cases where DNSSEC validation applies, of course.

The behavior is probably best understood through the fact
that filters and actions are completely decoupled.
The forwarding actions have no clue about why they were executed,
e.g. that the user wanted to restrict the forwarder only to some subtree.
The action just selects some set of forwarders to process this whole request from the client,
and during that processing it might need some other "sub-queries" (e.g. for validation).
Some of those might not've passed the intended filter,
but policy rule-set only applies once per client's request.

.. _tls-forwarding:

Expand Down

9 comments on commit ccb9d97

@idealeer
Copy link

Choose a reason for hiding this comment

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

It indeed is a potential issue, and we have conducted the actual cache poisoning attack with this issue.

Everyone using the filter policy.suffix should notice this issue.

Moreover, we hope Knot will fix the issue in the next major version as soon as possible.

@vcunat
Copy link
Member Author

@vcunat vcunat commented on ccb9d97 Jan 11, 2022

Choose a reason for hiding this comment

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

For reference, we have a huge ticket the policy rewrite, and I believe that the current plans of the design will also mitigate this (incidentally): https://gitlab.nic.cz/knot/knot-resolver/-/issues/535

@idealeer
Copy link

Choose a reason for hiding this comment

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

Great! When do you guys plan to implement the new designed policy modules?

@vcunat
Copy link
Member Author

@vcunat vcunat commented on ccb9d97 Jan 11, 2022

Choose a reason for hiding this comment

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

We have an old prototype, and I certainly plan to spend lots of time on this in the following few months.

@vcunat
Copy link
Member Author

@vcunat vcunat commented on ccb9d97 Jan 11, 2022

Choose a reason for hiding this comment

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

The docs change was merged, by the way.

@idealeer
Copy link

Choose a reason for hiding this comment

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

Have noticed it in the latest document version. Thanks for your assistance.

@idealeer
Copy link

Choose a reason for hiding this comment

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

Hi, have you guys started the rewriting process?

@vcunat
Copy link
Member Author

@vcunat vcunat commented on ccb9d97 May 31, 2022

Choose a reason for hiding this comment

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

Started? Well, yes I could say that. But in the past couple of months it got stalled due to more immediate concerns taking more work.

Anyway, you can follow that branch: https://gitlab.nic.cz/knot/knot-resolver/-/compare/master...new-policy

@idealeer
Copy link

Choose a reason for hiding this comment

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

nice

Please sign in to comment.