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

Netgoblin fork #103

Open
wants to merge 62 commits into
base: master
Choose a base branch
from
Open

Netgoblin fork #103

wants to merge 62 commits into from

Conversation

warsang
Copy link
Contributor

@warsang warsang commented May 17, 2017

The Netgoblin fork is part of a Conix project which aims to create a tool for assisting protocol inference.
Netgoblin is intended to work with the TAPIRE CLI another part of the project which has not been open-sourced yet (as it is not yet fully developed).
The Netgoblin fork features:

  • Several "bug" corrections :
  1. TypeEncodingFunctions + ASCII conversion would delete data (refer to warsang@09ae2dd).
  2. Support for the Alt domain field in the Value class
  3. Support for the Alt domain field in the InternetChecksum
  4. Cells displayed in ISO-8859-1
  • CRC32 domain field + doctests
  • CRC32 Seeker and Field creation + doctests
  • CRC32 symbol clustering (lacks doctests for now)
  • IP seeker and Field creation + doctests
  • Size seeker capable of finding inside field Size relations + automatic Field creation + doctests
  • HeaderDetector + doctests
  • Value persisten SVAS, allows operations such as incremental field specialisation (see doctests)
  • RelationFinder now supports data and equality relations
  • Session, Source IP and Destination IP in symbol display
  • Session added to symbol in PCAP importer

-Added core files. Main differences with current netzob is correction of two bugs(see README) as well as CRC32 domain for fields.
…n AbstractMessage.py.

The data was decoded from utf-8 before being printed. Removed this as python3.5 supports printing raw bit strings.
The issue was that Value had trouble with Alt Fields. Did a little workaround that works for now in the Value.py file
…ate Checksums from Alt fields. Added a .gitignore
This merge integrates the crc32 Relation wich now works with Alt fields as well. It also fixes Value and InternetChecksum Relations wich had similar issues
…mps to create a CRCField.

The automatic field creation is not implemented yet but I have finsished a function capable (most of the time) of taking as input a message index and returning which field the index belongs to.
…ugging purposes.

Same goes for CRC32.py (I think).
CRCFinder.py added automated field creator. It now works on fixed length fields. Has been tested on a field containing a LE CRC. The field was the size of the CRC32(4bytes).
It was a CRC computed thanks to all the bytes that followed the CRC
-CRCFinder.py Added stuff to make the above test work. Not sure how it works but it does!
-CRCFinder.py: Tested the CRC_mid_BE field creation. Seems to work all right. Index in search_mid_crc method was wrong. Fixed it!
This is the Netgoblin Beta Version.
It includes:
-Several bug fixes from Netzob (especially in relation Fields)
-A CRC32 domain relation field
-A CRC32 seeker
-IPseeker.py:  The new IPseeker class.
-Deleted all *.pyc. I hope my gitignore will ignore them now.
The errors give me the same expected and actual output.
-Added a getFieldFromIndex method to symbol.py
-IPseeker.py: Can now specify if you wish to search for two termed IPs or not
-CRCFinder.py: Just some debugging modifications from playing around
-ClusterByCRC.py: The clusterer class. Can cluster raw messages into symbols or from symbol messages
-Format.py: Added the Clusterer to the Format class. Also removed a useless import.
-Value.py : Added the operation property
-AbstractVariable.py: No real changes
-BitArray.py: No major changes
-FieldSpecializer.py: No major changes
-AbstractRelationVariableLeaf.py: Added the regenerateandmemorize method
-AbstractVariableLeaf.py: Change the call to self.use(...) to self.regenerateandmemorize(...)
-Value.py: Added the generate() method
-Field.py: Added the SpecializingPaths as a property. It is kept persistent every time svas is PERSISTENT
-ClusterByCRC.py: Now accepts messages TypedList as input,if input is symbol, retrieves the old name to creat the new names
-HeaderDetector.py: Several methods implemented to separate header and data: ratio based, separator based, domain relation based
-RelationFinder.py: Implemented netzob#87 and added the equalityRelation
-IPFinder.py: Just renamed and changed a spelling mistake in a comment
-CRCFinder.py: Same
-all.py: Added the SizeFinder
-SizeFinder.py: Implemented a method to find size inside fields and create new fields.
-Size.py: Just changed a comment
warsang added 10 commits May 16, 2017 17:13
-CRCFinder.py: Tested field creation and normal CRC32 LE detection as well as mid CRC32LE detection
-headerDetector.py: Tested Field type detection and  value separator detection
-CRCFinder.py: Added doctest
-IPFinder.py: Added doctest and changed call to message.l3DestinationAddress to message.destination[:-5] (so that it also works with RawMessages)
-SizeFinder.py: Added doctests and deleted a print('\n')
-headerDetector.py: Added doctests
-Deleted several empty __init__.py files created by IDE
The netgoblin project is now up to date with netzob. This release can be considered the first official stable release
@codecov-io
Copy link

codecov-io commented May 17, 2017

Codecov Report

Merging #103 into next will increase coverage by 0.64%.
The diff coverage is 60.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #103      +/-   ##
==========================================
+ Coverage   68.54%   69.18%   +0.64%     
==========================================
  Files         157      159       +2     
  Lines        9484    10694    +1210     
==========================================
+ Hits         6501     7399     +898     
- Misses       2983     3295     +312
Impacted Files Coverage Δ
netzob/src/netzob/Inference/Grammar/GenericMAT.py 0% <ø> (ø) ⬆️
netzob/src/netzob/Model/Vocabulary/Types/Raw.py 85.13% <ø> (ø) ⬆️
.../Vocabulary/Domain/Specializer/FieldSpecializer.py 94.28% <ø> (ø) ⬆️
.../netzob/Model/Vocabulary/Domain/Specializer/all.py 100% <ø> (ø) ⬆️
...ocabulary/Domain/Specializer/MessageSpecializer.py 76.69% <ø> (ø) ⬆️
...unctions/EncodingFunctions/ZLibEncodingFunction.py 96.96% <ø> (ø) ⬆️
...ob/Model/Grammar/Transitions/AbstractTransition.py 83.33% <ø> (-0.5%) ⬇️
netzob/src/netzob/Common/Utils/SortableObject.py 75% <ø> (ø) ⬆️
...odel/Grammar/Transitions/CloseChannelTransition.py 51.72% <ø> (ø) ⬆️
.../Vocabulary/Domain/Specializer/SpecializingPath.py 95.23% <ø> (ø) ⬆️
... and 125 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5006b8d...6be0c5d. Read the comment docs.

@warsang warsang mentioned this pull request May 19, 2017
-netzob/src/netzob/Export/WiresharkExporter.py: A trivial wireshark dissector exporter
-netzob/src/netzob/Export/all.py: Import call to the WiresharkExporter
-netzob/src/netzob/Import/__init__.py:deleted empty file created by IDE
-netzob/src/netzob/__init__.py: Removed a useless line created by an old merge
-netzob/src/netzob/all.py: Added import call to Export modules
Copy link
Member

@Sygus Sygus 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 important PR.

As a first comment, it seems there are some duplicated code, maybe due to a parallel PR that provided code refactoring. I'll try to point out some of them.

@@ -67,6 +68,9 @@ class LoggingConfiguration(object):
#+----------------------------------------------
def __init__(self, workspace, opts):
# First we extract the normal logging config file
self.loggingFilePath = os.path.join(workspace.getPath(), workspace.getPathOfLogging())
Copy link
Member

Choose a reason for hiding this comment

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

Code duplicate ?

@@ -80,13 +84,15 @@ def __init__(self, workspace, opts):
logger = logging.getLogger("")
logger.setLevel(logging.INFO)
h = logging.StreamHandler()
f = logging.Formatter("[%(threadName)s]%(asctime)s - %(module)s - %(levelname)s - %(message)s")
Copy link
Member

Choose a reason for hiding this comment

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

Code duplicate ?

@@ -115,6 +121,8 @@ def setLoggingLevel(self, level):
logger = logging.getLogger("")

if level in ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']:
logging.info("Updating logging level from {0} to {1}".format(logging.getLevelName(logger.level),
Copy link
Member

Choose a reason for hiding this comment

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

Code duplicate ?

@@ -95,6 +95,8 @@ class ParallelDataAlignment(object):
>>> autoThreadDuration = end-start
>>> print(len(alignedData))
1000
>>> autoThreadDuration <= oneThreadDuration
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to remove the tiny test, as it breaks Travis CI builds (as Travis builds work on one thread).

@@ -85,6 +85,7 @@ def emit(self, record):
if not self.is_tty:
self.stream.write(message)
else:
self.stream.write(self.colours[record.levelname] + message + Style.RESET_ALL)
Copy link
Member

Choose a reason for hiding this comment

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

Code duplicate ?




return "TEST"
Copy link
Member

Choose a reason for hiding this comment

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

Code duplicate ?

@@ -73,6 +73,7 @@ def __init__(self, membersTypes, *args):

def check(self, v):
if not isinstance(v, self.membersTypes):
raise TypeError("Invalid type for argument, expecting: {0}, received : {1}".format(self.membersTypes, v.__class__.__name__))
Copy link
Member

Choose a reason for hiding this comment

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

Code duplicate ?

@@ -35,7 +35,7 @@
try:
import pcapy
pcapy_available = True
except ImportError:
except ImportError:
Copy link
Member

Choose a reason for hiding this comment

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

There some areas where unecessary spaces are added at the end of the line in this PR (maybe because of the underlying editor ?). I would suggest to use a tool (such as autopep8, yapf, or pep8) to remove uncessary spaces.

#| |
#| Netzob : Inferring communication protocols |
#+---------------------------------------------------------------------------+
#| Copyright (C) 2011-2017 Georges Bossert and Frédéric Guihéry |
Copy link
Member

Choose a reason for hiding this comment

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

This is an obsolete class not in the current 'next' branch of netzob. I guess you have merged your work with the 'master' branch or an old 'next' branch version. I would suggest to synchronize your PR branch with the current version of the 'next' branch.

@@ -0,0 +1,317 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Same remark. This is an obsolet class ;)

@warsang
Copy link
Contributor Author

warsang commented Jun 11, 2017

Yes, I did notice this. I have been struggling to get rid of it, however I am afraid I missed quite a bit. Nevertheless I don't believe it affects netzob functionality so testing the features should be ok. The few issues however are that it probably reduces performance and overall code coverage.

Moreover I am not sure how to merge this PR as it is quite big and has features which Netzob might not need(ex: Hex also converted to Non Ascii printable characters by TypeEncodingFunction). Gbossert suggested on IRC to do several smaller PR'S. I have only had the time to do two as of today.

If you find some code duplicates I'd be happy to correct them!

@Sygus
Copy link
Member

Sygus commented Jun 11, 2017

"Yes, I did notice this. I have been struggling to get rid of it"

I see at least two different ways to resolve this situation:

I clearly prefer the first one, as you will have a better control of what you're doing (even though it will take some time).

Sygus added a commit that referenced this pull request Dec 29, 2022
Resolve "Update Actor doctest"

Closes #103

See merge request ANS015/netzob!288
Sygus added a commit that referenced this pull request Dec 30, 2022
Resolve "Update Actor doctest"

Closes #103

See merge request ANS015/netzob!288
@Sygus Sygus closed this in cb29071 Jan 3, 2023
@Sygus Sygus reopened this Jan 3, 2023
@Sygus Sygus changed the base branch from next to master January 4, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants