Skip to content

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Apr 2, 2019

This PR:

  • cleanup DNSStrField calls by moving the encoding function to dns_encode
  • cleanup RDataField to be split into several fields, using MultipleTypeField
  • remove fields that have obvious replacements (TimeField -> UTCTimeField, RDLen -> FieldLenField+i2len)
  • make UTscapy work with both lower and upper case keywords (there are DNS and dns tags, that were annoying)
  • make UTscapy import Bunch and retry_test on all test campaigns => remove the imports in the tests

Edit:
The Unicode/UTF8 fix was removed from the PR, awaiting for a proper fix to be found

@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #1968 into master will decrease coverage by 5.35%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master    #1968      +/-   ##
==========================================
- Coverage   85.87%   80.51%   -5.36%     
==========================================
  Files         187      132      -55     
  Lines       43010    31277   -11733     
==========================================
- Hits        36933    25182   -11751     
- Misses       6077     6095      +18
Impacted Files Coverage Δ
scapy/fields.py 88.63% <100%> (-3.04%) ⬇️
scapy/layers/dns.py 92.69% <95.74%> (+0.97%) ⬆️
scapy/layers/tls/crypto/hkdf.py 35.55% <0%> (-64.45%) ⬇️
scapy/arch/pcapdnet.py 16.99% <0%> (-40.05%) ⬇️
scapy/layers/tls/record_tls13.py 29.72% <0%> (-30.64%) ⬇️
scapy/layers/usb.py 51.64% <0%> (-29.68%) ⬇️
scapy/arch/common.py 66.3% <0%> (-25%) ⬇️
scapy/supersocket.py 51.16% <0%> (-20.94%) ⬇️
scapy/consts.py 70.83% <0%> (-20.84%) ⬇️
scapy/layers/tls/keyexchange_tls13.py 33.87% <0%> (-20.77%) ⬇️
... and 98 more

@gpotter2 gpotter2 force-pushed the dnsfix branch 3 times, most recently from 044c058 to 458a2fe Compare April 2, 2019 21:46
@arnout
Copy link

arnout commented Apr 3, 2019

One more remark:

>>> dns_compress(DNS(an = (DNSRR(rrname='a.') / DNSRR(rrname='b.a.'))))
<DNS  an=<DNSRR  rrname='a.' |<DNSRR  rrname='\x01b\xc0\x0c' |>> |>

i.e. no decompression is done when showing the result of dns_compress, while it is done when constructing of from a raw packet. That was already the case before this patch, though. I just mention this because it might also be fixable by playing with any2i.

@gpotter2
Copy link
Member Author

gpotter2 commented Apr 3, 2019

That’s expected. Performing .show2() will show it decompressed. Otherwise it wouldn’t be compressed while building

@gpotter2
Copy link
Member Author

gpotter2 commented Apr 3, 2019

Could we get #1849 merged before I start conflicting with it ? Thanks !

@p-l-
Copy link
Member

p-l- commented Apr 3, 2019

@gpotter2 yep, done. Sorry it took so long.

@gpotter2
Copy link
Member Author

gpotter2 commented Apr 3, 2019

I’m thinking of replacing RDataField with a MultipleTypefield. We could use the IP’s fields + the DNSStrField + a custom field to handle text encoding, therefore fix #1968 (comment) + cleanup at the same time.

It’s only used once in Scapy, and will probably be much cleaner. Will look into that when I can

@p-l-
Copy link
Member

p-l- commented Apr 3, 2019

@gpotter2 that could be a good idea!

@gpotter2 gpotter2 added the cleanup Performs some code clean-up label Apr 4, 2019
@gpotter2 gpotter2 force-pushed the dnsfix branch 2 times, most recently from 688ab87 to e17b689 Compare April 4, 2019 22:25
@gpotter2 gpotter2 changed the title Fix DNS already-encoded detection DNS cleanup & various cleanups Apr 5, 2019
@gpotter2
Copy link
Member Author

gpotter2 commented Apr 6, 2019

@p-l- @guedou Ready to be reviewed. The PR shifted from its original intent to simply focus on general cleanups. I reverted the encoding fix, as it was probably wrong in the first place

Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

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

Good to be merged. @p-l- could you have a look? As it involves DNS, I think that it is safe to have another review =)

@gpotter2 gpotter2 requested a review from p-l- April 20, 2019 13:01
@gpotter2 gpotter2 mentioned this pull request Apr 23, 2019
27 tasks
@p-l- p-l- merged commit 78481fa into secdev:master Apr 26, 2019
@gpotter2 gpotter2 deleted the dnsfix branch April 26, 2019 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Performs some code clean-up

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants