Skip to content

Fix lldp org specific dissection clean #1151#1154

Merged
guedou merged 7 commits intosecdev:masterfrom
speakinghedge:fix-lldp-org-specific-dissection-clean-#1151
Mar 9, 2018
Merged

Fix lldp org specific dissection clean #1151#1154
guedou merged 7 commits intosecdev:masterfrom
speakinghedge:fix-lldp-org-specific-dissection-clean-#1151

Conversation

@speakinghedge
Copy link
Copy Markdown
Contributor

  • adds a new field ThreeBytesEnumField
  • decodes organisation specific TLVs in a generic way
  • ensure that if something unexpected appears the remaining payload is put into a Raw() layer

fixes #1151

required to show the names of the
organizations used in LLDP org specific TLVs
implements generic encoding of org specific TLVs and
dumps the payload as hex-string
Copy link
Copy Markdown
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.

Thanks for this PR. Please see my comments.

Comment thread scapy/contrib/lldp.py
def guess_payload_class(self, payload):
# type is a 7-bit bitfield spanning bits 1..7 -> div 2
lldpdu_tlv_type = orb(payload[0]) // 2
return LLDPDU_CLASS_TYPES[lldpdu_tlv_type]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use this instead:
return LLDPDU_CLASS_TYPES.get(lldpdu_tlv_type, Raw)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addressed in c2aea83

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May I suggest to use conf.raw_layer instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 , see d7017d9

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

while reading more code that forms the basement of scapy I came across default_payload_class() in class packet . should/could this be used here instead of
conf.raw_layer or would this generate to much indirection, abstraction, ... ?

Comment thread scapy/fields.py Outdated
return lhex(x)


class ThreeBytesEnumField(EnumField, ThreeBytesField):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Keep it lldp.uts as it not used elsewhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addressed in 7578ef2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

uhm.. did you mean test or test & code?

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 16, 2018

Codecov Report

Merging #1154 into master will increase coverage by 0.57%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   83.47%   84.05%   +0.57%     
==========================================
  Files         157      159       +2     
  Lines       37855    38225     +370     
==========================================
+ Hits        31599    32129     +530     
+ Misses       6256     6096     -160
Impacted Files Coverage Δ
scapy/fields.py 90.26% <ø> (-0.01%) ⬇️
scapy/contrib/lldp.py 93.05% <88.23%> (-0.42%) ⬇️
scapy/layers/can.py 78.04% <0%> (-21.96%) ⬇️
scapy/layers/netflow.py 94.73% <0%> (-2.14%) ⬇️
scapy/contrib/mpls.py 94.11% <0%> (-1.34%) ⬇️
scapy/error.py 88.88% <0%> (-1.31%) ⬇️
scapy/arch/pcapdnet.py 68.77% <0%> (-0.86%) ⬇️
scapy/config.py 85.12% <0%> (-0.09%) ⬇️
scapy/data.py 83.48% <0%> (-0.08%) ⬇️
... and 17 more

@guedou guedou self-assigned this Mar 9, 2018
@guedou guedou merged commit 71ac357 into secdev:master Mar 9, 2018
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.

lldp fails to decode messages containing organisation specific options

5 participants