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

Rpz #73

Merged
merged 59 commits into from
Jan 30, 2020
Merged

Rpz #73

merged 59 commits into from
Jan 30, 2020

Conversation

ralphdolmans
Copy link
Contributor

No description provided.

bring fork up-to-date with upstream
- Fixes for compiler warnings
 - Added RPZ policy apply logging
 - Fix memory leak
 - IANA ports update
 - merge littlehash ASAN changes
- Fix potential memory leak
- Fix rpz memory leak
- Fix memory leak
 - use localzone's memory layout when removing rr from rrset
@spirillen
Copy link

What a shame All checks have failed I'm still hoping you prioritize this MR as it would move you way up to the top of my choice of selection in recursors 😃

Comment on lines 378 to 379
if((enum rpz_action)s->svr.rpz_action[i] == RPZ_NO_OVERRIDE_ACTION)
continue;
Copy link
Member

Choose a reason for hiding this comment

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

This must be wrong, the value is a statistics counter, it is never assigned to this variable and the enum is a valid value for the statistics counter.

Copy link
Member

Choose a reason for hiding this comment

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

It is also wrong in remote.c, where the same line is copied from.

services/rpz.c Outdated
Comment on lines 121 to 125
if(*dname == 0)
return NULL;

while(*dname) {
dname = dname+*dname+1;
dnamelen += ((size_t)*dname)+1;
Copy link
Member

Choose a reason for hiding this comment

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

The if(dnamelen > maxdnamelen) check needs to be before the *dname parts above, in the if(..==0) and while() and in the dnamelen+= statements, you first need to check that the label length byte fits in the maxdnamelen. With an added statement like if(dnamelen+1 > maxdnamelen) return NULL; at the start of the routine. I don't think it is needed at the end of the while loop.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think it is also needed before the *dname in the != 0 comparison below, an if statement to check if dnamelen+1 > maxdnamelen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack on the check at the start of the routine, in case of a maxdnamelen of 0.

For the second check (in the while) I don't think a new if is needed, changing the existing one from if(dnamelen > maxdnamelen) to if(dnamelen+1 > maxdnamelen) should do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See ef12073

services/rpz.c Outdated Show resolved Hide resolved
services/rpz.c Outdated Show resolved Hide resolved
if(dname_is_wild(ctarget)) {
/* synthesize cname target */
struct packed_rrset_data* d;
uint8_t newtarget[LDNS_MAX_DOMAINLEN];
Copy link
Member

Choose a reason for hiding this comment

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

Other buffers are LDNS_MAX_DOMAINLEN+1. Also for rpz.c:646 wc[LDNS_MAX_DOMAINLEN], the +1 is elsewhere but not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The newtarget[] is actually not used at all and removed in a later commit (344f12d)

Copy link
Contributor Author

@ralphdolmans ralphdolmans Jan 29, 2020

Choose a reason for hiding this comment

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

added +1 for the wc[] in rpz.c (7da16fe)

util/net_help.c Outdated
Comment on lines 303 to 304
len = *dname;
lablen = *dname++;
Copy link
Member

Choose a reason for hiding this comment

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

ipdnametoaddr and netblockdnametoaddr also have dnamelen == 0 check needed. And perhaps also check if labellengthbyte fits in dnamelen, and if the label length fits in dnamelen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 88a706a

Also subtracting the netblock label (incl. length byte) from length passed to ipdnametoaddr.

Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Reviewed (change commits since previous), and it looks good! (I think this comment allows later github diffs to be made as well, hence).

@ralphdolmans ralphdolmans merged commit b9c9fc0 into master Jan 30, 2020
@ralphdolmans ralphdolmans deleted the rpz branch January 30, 2020 15:07
@spirillen
Copy link

spirillen commented Jan 30, 2020

👏 👏 👏 👏 👏

Is there are release date for this? like 1.9.7?

@ralphdolmans
Copy link
Contributor Author

Is there are release date for this? like 1.9.7?

This will be part of the upcoming Unbound 1.10 release, which is scheduled to happen in February.

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 this pull request may close these issues.

4 participants