Skip to content
Snippets Groups Projects

Kzonecheck DNSSEC improvements

Merged Kzonecheck DNSSEC improvements
kzonecheck into master
Merged Filip Siroky requested to merge kzonecheck into master

Add optional check for different time. Fix error messages. Add new checks.

closes #501 (closed)

Edited by Daniel Salzman

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Filip Siroky marked as a Work In Progress

    marked as a Work In Progress

  • Filip Siroky added 3 commits

    added 3 commits

    • 7e614684 - kzonecheck: typo fix
    • 88f091e6 - kzonecheck: fix wrong error message
    • 73785108 - kzonecheck: add NSEC3 error for incorrect type bitmap

    Compare with previous version

  • Filip Siroky closed

    closed

  • Filip Siroky reopened

    reopened

  • Filip Siroky unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Libor Peltan
    Libor Peltan @lpeltan ·
    Maintainer

    Some comments:

    adjust_wire_ttl() seems like re-inventing wheel to me, how about using knot_rdata_set_ttl() or so, to prevent having doubled code ?

    similar for #define RRSIG_RDATA_SIGNER_OFFSET 18 - don't we have some parsing tool for this in libknot ? Can we consider adding some? From code/comments it's not clear what type of RR has this structure and if we check that we got the desired type.

    anyway, i slightly dislike the idea of manipulating rrset after wiring it. Let me think of the design more after reading correcponding RFCs.

    Finally, there are some things odd:

    assert(sizeof(uint16_t) == 2);
assert(sizeof(uint32_t) == 4);

    ...you don't have to care.

    	dnssec_binary_t header = { 0 };
	header.data = (uint8_t *)rdata;
	header.size = RRSIG_RDATA_SIGNER_OFFSET;

    ...you can just:

    const dnssec_binary_t header = { .data = rdata, .size =  RRSIG_RDATA_SIGNER_OFFSET };

    and so on.

    #define		SHA1	1
#define		SHA256	2
#define		GOSTR	3
#define		SHA384	4

    ...better use DNSSEC_KEY_DIGEST_SHA384 and similar from ./src/dnssec/lib/dnssec/key.h.

    To be continued....

  • Libor Peltan
    Libor Peltan @lpeltan ·
    Maintainer

    Please read https://gitlab.labs.nic.cz/knot/resolver/blob/master/lib/dnssec.c and think of, if we (partly?) moved this resolver code into libknot, would it help us to make check_signature() trivial and based on well-tested backend. Bud only(!) in case the resolver part does not use too much resolver-specific stuff.

    Continuation of comments:

    		if (ret != KNOT_EOK) {
			dnssec_nsec3_params_free(&params_apex);
			free(owner);
			return ret;
		}

    (multiple times) ...we usually tend to use linux-kernel style of error handling:

    if (ret != KNOT_EOK) {
goto err_label;
}
...
err_label:
dnssec_nsec3_params_free(&params_apex);
free(owner);
return ret;

    (also using that free(NULL); makes no problems) ...alternatively (maybe better here) declare a macro doing all the checks, cleanup and return and make the check one-line.

    uint8_t param = knot_nsec3param_salt_length(nsec3param, 0);
				if (param > 255) {

    WTF ???

    param = knot_nsec3param_algorithm(nsec3param, 0);
				if (param != 1) {

    better use symbolic constant for algorithm instead of 1

    Better move long code from zone_do_sem_checks() into separated functions.

    For the NSEC3 error handler function, it would be better to have argument for const knot_dname_t *owner to avoid 1. too many knot_dname_to_str_alloc, 2. cryptic using "owner" for "hash".

    Still, to be continued ;) Hope you are not dismotivated yet.

  • Filip Siroky
    Filip Siroky @fsiroky ·
    Author Contributor

    Not at all, valid points are always welcomed :) Agreed, the code is duplicate and I will look into it.

  • Libor Peltan
    Libor Peltan @lpeltan ·
    Maintainer

    I would prefer an enum instead of bool severity. Is there any existing enum somewhere around to be re-used?

    "trim terminating dot" ... normally we keep the dot in logs. See any log. One of reasons: logging root zone. If the dot has been trimmed before, no problem with it, but you can consider this.

    sematic-check.c:

    l.269 remove

    l.337 s/0/KNOT_EOK/

    l.366 useless ?

    l.388, 389 split, even if it is not pretty

    check_rrsig_rdata()/dnskey check ... do we check that dnskeys are rrsigned with ksk and other records with zsk ? Maybe bool *flag does this, but a weird way. Moreover, use DNSKEY_FLAGS_ZSK instead of (1 << 8) (I don't get the point completely, because KSK is 257 and ZSK is 256, so (bool)(KSK | ZSK) == true) (even though it possibly makes conflict with kasp_refactor soon to be merged)

    l. 581, 582 indentation

    l. 661..682 better use a table? const uint16_t digest_sizes[] = { 0xffff, 20, 32, 48, 32 }; if (digest_size != digest_sizes[digest_type]) ..., avoid too much of continue;

    l. 1214 s/OPTOUT/NSEC3_PARAM_OPTOUT/

    l. 1232 instead of 1 use a symbolic constant named by its meaning

    l. 1259 meaning of 3 ?

    l. 1275..1276 substitute with } else {

    Please tell me if dnssec validation is default for server zone load, or not. I would better leave this just for kzonecheck utility.

  • Libor Peltan
    Libor Peltan @lpeltan ·
    Maintainer

    Clang:

      CC       knot/zone/libknotd_la-semantic-check.lo
knot/zone/semantic-check.c:1053:6: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
        if (next_dname == NULL) {
            ^~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:1053:2: note: remove the 'if' if its condition is always false
        if (next_dname == NULL) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1037:7: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                if (ret != KNOT_EOK) {
                    ^~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:1037:3: note: remove the 'if' if its condition is always false
                if (ret != KNOT_EOK) {
                ^~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1029:7: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                if (ret != KNOT_EOK) {
                    ^~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:1029:3: note: remove the 'if' if its condition is always false
                if (ret != KNOT_EOK) {
                ^~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1021:7: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                if (ret != KNOT_EOK) {
                    ^~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:1021:3: note: remove the 'if' if its condition is always false
                if (ret != KNOT_EOK) {
                ^~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1008:7: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                if (ret != KNOT_EOK) {
                    ^~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:1008:3: note: remove the 'if' if its condition is always false
                if (ret != KNOT_EOK) {
                ^~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1001:6: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
        if (ret != DNSSEC_EOK) {
            ^~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:1001:2: note: remove the 'if' if its condition is always false
        if (ret != DNSSEC_EOK) {
        ^~~~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:989:7: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                if (ret != KNOT_EOK) {
                    ^~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:989:3: note: remove the 'if' if its condition is always false
                if (ret != KNOT_EOK) {
                ^~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:976:6: warning: variable 'hash_info' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
        if (nsec3_rrs == NULL) {
            ^~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:1088:7: note: uninitialized use occurs here
        free(hash_info);
             ^~~~~~~~~
knot/zone/semantic-check.c:976:2: note: remove the 'if' if its condition is always false
        if (nsec3_rrs == NULL) {
        ^~~~~~~~~~~~~~~~~~~~~~~~
knot/zone/semantic-check.c:961:17: note: initialize the variable 'hash_info' to silence this warning
        char *hash_info;
                       ^
                        = NULL
8 warnings generated.

    Valgind for basic/cname_follow:

    ==16910== 264 bytes in 8 blocks are definitely lost in loss record 1 of 1
==16910==    at 0x4C2DB8F: malloc (vg_replace_malloc.c:299)
==16910==    by 0x4C2FDEF: realloc (vg_replace_malloc.c:785)
==16910==    by 0x4A38AC: mm_realloc (mempattern.c:57)
==16910==    by 0x47C844: add_rr_at (rdataset.c:69)
==16910==    by 0x47C76C: knot_rdataset_add (rdataset.c:235)
==16910==    by 0x433BB5: knot_synth_rrsig (rrset-sign.c:316)
==16910==    by 0x45DBAC: check_rrsig_in_rrset (semantic-check.c:558)
==16910==    by 0x45C804: check_rrsig (semantic-check.c:722)
==16910==    by 0x45BC3F: do_checks_in_tree (semantic-check.c:1207)
==16910==    by 0x459981: tree_apply_cb (contents.c:45)
==16910==    by 0x4AA541: apply_trie (trie.c:762)
==16910==    by 0x4AA596: apply_trie (trie.c:765)

    for dnssec/case_sensitivity

    ==17641== 97 bytes in 1 blocks are definitely lost in loss record 1 of 1
==17641==    at 0x4C2DB8F: malloc (vg_replace_malloc.c:299)
==17641==    by 0x4C2FDEF: realloc (vg_replace_malloc.c:785)
==17641==    by 0x4A38AC: mm_realloc (mempattern.c:57)
==17641==    by 0x47C844: add_rr_at (rdataset.c:69)
==17641==    by 0x47C76C: knot_rdataset_add (rdataset.c:235)
==17641==    by 0x433BB5: knot_synth_rrsig (rrset-sign.c:316)
==17641==    by 0x45DBAC: check_rrsig_in_rrset (semantic-check.c:558)
==17641==    by 0x45C804: check_rrsig (semantic-check.c:722)
==17641==    by 0x45BC3F: do_checks_in_tree (semantic-check.c:1207)
==17641==    by 0x459981: tree_apply_cb (contents.c:45)
==17641==    by 0x4AA541: apply_trie (trie.c:762)
==17641==    by 0x4AA596: apply_trie (trie.c:765)

    unwanted printf makes the output confusing:

    peltan@peltan:~/master_knot/src$ ./kzonecheck -t 1491497053 ~/conf/1/example.com.zone
opt 1491497053
peltan@peltan:~/master_knot/src$

    segfault of knotd with semantic-checks:on and zone with expired rrsigs...

    (gdb) r -c ~/3knot.conf 
Starting program: /home/peltan/master_knot/src/knotd -c ~/3knot.conf
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
2017-04-06T19:00:08 info: Knot DNS 2.5.0-dev starting
2017-04-06T19:00:08 info: binding to interface '0.0.0.0@38828'
2017-04-06T19:00:08 info: loading 1 zones
2017-04-06T19:00:08 info: [example.com.] zone will be loaded, serial none
2017-04-06T19:00:08 info: starting server
[New Thread 0x7fffcfaa4700 (LWP 25024)]
2017-04-06T19:00:08 warning: [example.com.] semantic check, record 'example.com.', expired RRSIG, record type 'SOA'
2017-04-06T19:00:08 warning: [example.com.] semantic check, record 'example.com.', expired RRSIG, record type 'NS'
2017-04-06T19:00:08 warning: [example.com.] semantic check, record 'example.com.', expired RRSIG, record type 'MX'
2017-04-06T19:00:08 warning: [example.com.] semantic check, record 'example.com.', expired RRSIG, record type 'NSEC3PARAM'
2017-04-06T19:00:08 warning: [example.com.] semantic check, record 'example.com.', expired RRSIG, record type 'NSEC3PARAM'
2017-04-06T19:00:08 warning: [example.com.] semantic check, record 'example.com.', expired RRSIG, record type 'DNSKEY'

Thread 2 "knotd" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffcfaa4700 (LWP 25024)]
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x000000000045d21b in check_nsec3 (node=0x7fffc8006010, data=0x7fffcfa713a8) at knot/zone/semantic-check.c:1071
#2  0x000000000045bc40 in do_checks_in_tree (node=0x7fffc8006010, data=0x7fffcfa713a8) at knot/zone/semantic-check.c:1207
#3  0x0000000000459982 in tree_apply_cb (node=0x7fffc8005eb8, data=0x7fffcfa71350) at knot/zone/contents.c:45
#4  0x00000000004aa542 in apply_trie (t=0x7fffc8005eb0, f=0x459930 <tree_apply_cb>, d=0x7fffcfa71350) at contrib/qp-trie/trie.c:762
#5  0x00000000004aa597 in apply_trie (t=0x7fffc8006680, f=0x459930 <tree_apply_cb>, d=0x7fffcfa71350) at contrib/qp-trie/trie.c:765
#6  0x00000000004aa597 in apply_trie (t=0x7fffc8008c30, f=0x459930 <tree_apply_cb>, d=0x7fffcfa71350) at contrib/qp-trie/trie.c:765
#7  0x00000000004aa597 in apply_trie (t=0x7fffc8006080, f=0x459930 <tree_apply_cb>, d=0x7fffcfa71350) at contrib/qp-trie/trie.c:765
#8  0x00000000004aa597 in apply_trie (t=0x7fffc8005db0, f=0x459930 <tree_apply_cb>, d=0x7fffcfa71350) at contrib/qp-trie/trie.c:765
#9  0x00000000004aa4c3 in trie_apply (tbl=0x7fffc8005db0, f=0x459930 <tree_apply_cb>, d=0x7fffcfa71350) at contrib/qp-trie/trie.c:774
#10 0x0000000000461cfd in zone_tree_apply (tree=0x7fffc8005db0, function=0x459930 <tree_apply_cb>, data=0x7fffcfa71350) at knot/zone/zone-tree.c:204
#11 0x0000000000459919 in zone_contents_apply (contents=0x7fffc8005d10, function=0x45bb30 <do_checks_in_tree>, data=0x7fffcfa713a8) at knot/zone/contents.c:1051
#12 0x000000000045b7d2 in zone_do_sem_checks (zone=0x7fffc8005d10, optional=true, handler=0x7fffcfa714e0, context=1491498008) at knot/zone/semantic-check.c:1320
#13 0x0000000000427fd0 in zonefile_load (loader=0x7fffcfa71538) at knot/zone/zonefile.c:268
#14 0x0000000000460c71 in zone_load_contents (conf=0x7fffc80008c0, zone_name=0x731380 "\aexample\003com", contents=0x7fffcfaa3b00) at knot/zone/zone-load.c:54
#15 0x000000000043cc91 in event_load (conf=0x7fffc80008c0, zone=0x758c00) at knot/events/handlers/load.c:46
#16 0x000000000043b7b1 in event_wrap (task=0x758c90) at knot/events/events.c:196
#17 0x00000000004239c3 in worker_main (thread=0x7548d0) at knot/worker/pool.c:78
#18 0x000000000041f1d6 in thread_ep (data=0x7548d0) at knot/server/dthreads.c:161
#19 0x00007ffff70136ca in start_thread (arg=0x7fffcfaa4700) at pthread_create.c:333
#20 0x00007ffff6a440ff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105
(gdb)
  • Daniel Salzman marked as a Work In Progress

    marked as a Work In Progress

  • Filip Siroky added 332 commits

    added 332 commits

    • 4fdc90c5...ce8b3142 - 310 commits from branch master
    • 547a394c - kzonecheck: add option to change time for which is zone checked
    • 74bec1ea - kzonecheck: fix wrong error message
    • 9083cc37 - kzonecheck: add NSEC3 error for incorrect type bitmap
    • 51fa6683 - Knot DNS Deamon: add kzonecheck from utilities
    • 2ed8f174 - kzonecheck: add DS record check
    • 9e369cbf - kzonecheck: add enhanced NSEC3 chain error
    • af037865 - kzonecheck: add rrsig and dnskey check
    • a4f40355 - kzonecheck: add NSEC3 param check
    • d788678b - kzonecheck: fix semantic check test
    • 5561a826 - kzonecheck: add remaining rrsig checks (RFC4035) + minor fix and formating
    • 959db70f - tests-extra: new checks caused 2 test to fail, therefore were disabled
    • 5aafac89 - sem-check: regex test
    • 6be803fc - sem-check: regex test
    • 6e9a8b58 - sem-check: rework grep command to basic-regex and remove space ocuring only in NSEC3
    • fe934b93 - kzonecheck: add warning to semantic check, catch node with only RRsig and flag w…
    • d4efa4de - semantic-check: refactoring
    • 943f2dd2 - semantic-check: only one signature of rrset needs to be verified by key + test
    • bc9ea964 - kzonecheck: wip refactoring
    • 87bcda83 - kzonecheck: fix memory leak at rrsig check, using uninitialized variable, remove unwanted printf
    • ad5c2d4c - semantic-check: add NSEC3 logging function as it was missing for zonefile, split…
    • ca0d65f0 - semantic-check: severity into enum
    • 504815c2 - semantic-check: refactoring

    Compare with previous version

    Toggle commit list
  • Filip Siroky unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply