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

limit $GENERATE range to 2^16? #1019

Closed
miekg opened this issue Oct 3, 2019 · 11 comments · Fixed by #1020
Closed

limit $GENERATE range to 2^16? #1019

miekg opened this issue Oct 3, 2019 · 11 comments · Fixed by #1020

Comments

@miekg
Copy link
Owner

miekg commented Oct 3, 2019

a $GENERATE range that is too large will cause a panic, it make sense to limit this to something reasonable. I propose to limit the number of steps to 65535, i.e.

(stop - start) / step > 65535, and error out if we hit this.

@tmthrgd
Copy link
Collaborator

tmthrgd commented Oct 3, 2019

That sounds reasonable. What do other DNS servers do?

@miekg
Copy link
Owner Author

miekg commented Oct 3, 2019 via email

miekg added a commit that referenced this issue Oct 3, 2019
miekg added a commit that referenced this issue Oct 3, 2019
Having these checks means all test in TestCrasherString() are not
reached because we bail out earlier - removed that test all together.

Fixes #1019

Signed-off-by: Miek Gieben <[email protected]>
@chantra
Copy link
Contributor

chantra commented Oct 3, 2019

From: https://ftp.isc.org/isc/bind9/cur/9.11/doc/arm/Bv9ARM.ch06.html#generate_directive

range: This can be one of two forms: start-stop or start-stop/step. If the first form is used, then step is set to 1. start, stop and step must be positive integers between 0 and (2^31)-1. start must not be larger than stop.

@miekg
Copy link
Owner Author

miekg commented Oct 3, 2019 via email

@chantra
Copy link
Contributor

chantra commented Oct 3, 2019

this ooms my test (on my machine) - and it's a very large number for something
like this - not sure how to preceed. Use this number or something smaller?

it seems sensible to me to use something smaller. There is obviously no users of this feature with that many steps (or they would have reported it).
I think a sane limit is a simple fix. Supporting the bind behaviour may require a more substantial change which may not be worth it.

@miekg
Copy link
Owner Author

miekg commented Oct 3, 2019 via email

miekg added a commit that referenced this issue Oct 3, 2019
* Limit $GENERATE range to 65535 steps

Having these checks means all test in TestCrasherString() are not
reached because we bail out earlier - removed that test all together.

Fixes #1019

Signed-off-by: Miek Gieben <[email protected]>

* bring back testcase

Signed-off-by: Miek Gieben <[email protected]>

* bring back crash test

Signed-off-by: Miek Gieben <[email protected]>
@chantra
Copy link
Contributor

chantra commented Oct 22, 2019

and another flavor:

"$GENERATE 6-2048 $$GENERATE 6-036160 $$$$ORIGIN \\$"

this ends up calling generate on generate :)

@chantra
Copy link
Contributor

chantra commented Oct 22, 2019

diff --git a/scan.go b/scan.go
index 0f9361af..5567653b 100644
--- a/scan.go
+++ b/scan.go
@@ -732,6 +732,8 @@ type zlexer struct {
        nextL bool

        eol bool // end-of-line
+
+       generate bool
 }

 func newZLexer(r io.Reader) *zlexer {
@@ -886,6 +888,12 @@ func (zl *zlexer) Next() (lex, bool) {
                                case "$INCLUDE":
                                        l.value = zDirInclude
                                case "$GENERATE":
+                                       if zl.generate {
+                                               l.token = "$GENERATE can be used only once"
+                                               l.err = true
+                                               return *l, true
+                                       }
+                                       zl.generate = true
                                        l.value = zDirGenerate
                                }

would seem to work when dogsciencing but will likely break if there is 2 lines with the GENERATE directive.

@tmthrgd
Copy link
Collaborator

tmthrgd commented Oct 23, 2019

@chantra To prohibit $GENERATE in $GENERATE, it would be better to set a field on the sub parser created in generate.go. That won’t conflict with multiple $GENERATE directives in one zonefile like your patch will.

@chantra
Copy link
Contributor

chantra commented Oct 23, 2019

@tmthrgd I missed that.... I had tried to set it in the lexer or something... not the sub parser. diffing a fix.

@chantra
Copy link
Contributor

chantra commented Oct 23, 2019

@tmthrgd would this work? #1033

aanm pushed a commit to cilium/dns that referenced this issue Jul 29, 2022
* Limit $GENERATE range to 65535 steps

Having these checks means all test in TestCrasherString() are not
reached because we bail out earlier - removed that test all together.

Fixes miekg#1019

Signed-off-by: Miek Gieben <[email protected]>

* bring back testcase

Signed-off-by: Miek Gieben <[email protected]>

* bring back crash test

Signed-off-by: Miek Gieben <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants