scapy icon indicating copy to clipboard operation
scapy copied to clipboard

[Bluetooth] Add more bluetooth L2CAP packets

Open haramel opened this issue 4 years ago • 23 comments

Hello, I'm Haram Park. And I'm a Bluetooth security researcher at Computer & Communication Laboratory in Korea University(https://ccs.korea.ac.kr/). It was really useful for me to use scapy on my personal research. I really appreciate that. And I want to contribute to scapy.

During on my research, I need some Bluetooth L2CAP packets that were not embodied in scapy. So I modify bluetooth.py. I add some Bluetooth L2CAP packets in /scapy/scapy/layers/bluetooth.py. I refer to bluetooth core specification 5.2. (https://www.bluetooth.com/specifications/specs/core-specification/)

Add more Bluetooth L2CAP packets

  1. modify code list in L2CAP_CmdHdr()
  2. modify psm list in L2CAP_ConnReq()
  3. add Bluetooth L2CAP packets (1). L2CAP_EchoReq() (2). L2CAP_EchoResp() (3). L2CAP_Create_Channel_Request() (4). L2CAP_Create_Channel_Response() (5). L2CAP_Move_Channel_Request() (6). L2CAP_Move_Channel_Response() (7). L2CAP_Move_Channel_Confirmation_Request() (8). L2CAP_Move_Channel_Confirmation_Response() (9). L2CAP_LE_Credit_Based_Connection_Request() (10). L2CAP_LE_Credit_Based_Connection_Response() (11). L2CAP_Flow_Control_Credit_Ind() (12). L2CAP_Credit_Based_Connection_Request() (13). L2CAP_Credit_Based_Connection_Response() (14). L2CAP_Credit_Based_Reconfigure_Request() (15). L2CAP_Credit_Based_Reconfigure_Response()
  4. bind new packet with layers.

Thanks.

Haram Park

scapy_full_req

haramel avatar Jun 07 '21 11:06 haramel

Codecov Report

Merging #3250 (702e17f) into master (03bb705) will increase coverage by 32.47%. The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3250       +/-   ##
===========================================
+ Coverage   49.51%   81.98%   +32.47%     
===========================================
  Files         338      345        +7     
  Lines       75849    81484     +5635     
===========================================
+ Hits        37553    66808    +29255     
+ Misses      38296    14676    -23620     
Files Coverage Δ
scapy/layers/bluetooth.py 89.00% <100.00%> (+7.90%) :arrow_up:

... and 262 files with indirect coverage changes

codecov[bot] avatar Jun 07 '21 11:06 codecov[bot]

Hi & sorry for the delay !

This looks really good, however:

  • you need to add some tests (see test/scapy/layers/bluetooth.uts)
  • you have a bunch of pep8 failures
  • this has broken a test Once those issues are fixed, this should be good to go ! Thanks for your patience

gpotter2 avatar Jul 01 '21 01:07 gpotter2

Hi & sorry for the delay !

This looks really good, however:

  • you need to add some tests (see test/scapy/layers/bluetooth.uts)
  • you have a bunch of pep8 failures
  • this has broken a test Once those issues are fixed, this should be good to go ! Thanks for your patience
  • you need to add some tests (see test/scapy/layers/bluetooth.uts)
  • What should I add? Can you give me a example?

haramel avatar Jul 01 '21 08:07 haramel

You can build a packet and check the results of the raw bytes, or dissect bytes and check the results of the packet.

There are plenty of examples in the .uts files. Thanks

gpotter2 avatar Jul 01 '21 08:07 gpotter2

You can build a packet and check the results of the raw bytes, or dissect bytes and check the results of the packet.

There are plenty of examples in the .uts files. Thanks

I finished your feedback. However, I think something is wrong with test pipeline. I didn't modify import.uts, but in the continuous-integration/appveyor/pr log, there is a error with import.uts Please check this error. Thanks.

haramel avatar Jul 23 '21 10:07 haramel

image

Hello, this is Haram.

Could you please help me? Let me know what I have to change.. I didn't modify import.uts.. but there is a error...

I'm waiting for your answer..

Best regards, Haram Park

You can build a packet and check the results of the raw bytes, or dissect bytes and check the results of the packet.

There are plenty of examples in the .uts files. Thanks

haramel avatar Sep 02 '21 04:09 haramel

Hello ! This is probably unrelated, you can likely ignore it.

gpotter2 avatar Sep 02 '21 06:09 gpotter2

Hello ! This is probably unrelated, you can likely ignore it.

Ah, thanks for quick response. Then, what should I do now?

Haram Park.

haramel avatar Sep 02 '21 08:09 haramel

I had a look at the PR. It seems that some changes are only cosmetic. Could you revert them?

guedou avatar Sep 27 '21 09:09 guedou

I had a look at the PR. It seems that some changes are only cosmetic. Could you revert them?

I did, please check it. Thanks.

haramel avatar Sep 27 '21 14:09 haramel

The code have PEP08 related errors https://github.com/secdev/scapy/pull/3250/checks?check_run_id=3721770792#step:5:12

Could you fix them?

guedou avatar Sep 27 '21 14:09 guedou

The code have PEP08 related errors https://github.com/secdev/scapy/pull/3250/checks?check_run_id=3721770792#step:5:12

Could you fix them?

I fixed it, please check it..

haramel avatar Sep 28 '21 05:09 haramel

The code have PEP08 related errors https://github.com/secdev/scapy/pull/3250/checks?check_run_id=3721770792#step:5:12

Could you fix them?

I need your approval.

haramel avatar Oct 12 '21 11:10 haramel

The code have PEP08 related errors https://github.com/secdev/scapy/pull/3250/checks?check_run_id=3721770792#step:5:12

Could you fix them?

Since I need your approval every time, it takes a lot of time. Can't I proceed with the workflow without asking you for approval every time? This is because it is too inefficient to wait more than a week after modifying a few codes for pep8 error fix.

I need you approval one more, haha..^_^;;

Thanks, Haram Park

haramel avatar Oct 12 '21 12:10 haramel

You can run the PEP8 & mypy tests locally using

tox -e flake8,mypy

This would have been more efficient

gpotter2 avatar Oct 12 '21 13:10 gpotter2

tox -e flake8,mypy

I did it in local.. thanks! so now I don't have any pep8 error....!! I need your approval for workflow..! Check it please, thanks!

Haram Park

haramel avatar Oct 12 '21 14:10 haramel

You can run the PEP8 & mypy tests locally using

tox -e flake8,mypy

This would have been more efficient

Now, I have no more pep8 error.. However I got the wrong spelling and now I fixed it.. I need your approval.. check this please Thanks :)

Haram Park

haramel avatar Oct 13 '21 04:10 haramel

You can run the PEP8 & mypy tests locally using

tox -e flake8,mypy

This would have been more efficient

I'm sorry to bother you every time. I need your approval.

Thanks, Haram Park.

haramel avatar Oct 18 '21 07:10 haramel

You can run the PEP8 & mypy tests locally using

tox -e flake8,mypy

This would have been more efficient

I need your approval.

haramel avatar Oct 25 '21 14:10 haramel

The code have PEP08 related errors https://github.com/secdev/scapy/pull/3250/checks?check_run_id=3721770792#step:5:12

Could you fix them?

I need your approval! Thanks.

haramel avatar Nov 03 '21 15:11 haramel

@guedou I need your approval. Thanks

haramel avatar Nov 09 '21 16:11 haramel

@guedou I don't know why macos-10.15 3.9 both got error... I think it isn't related to my PR... anyway.. I need your approval..

haramel avatar Nov 10 '21 08:11 haramel

@guedou

Hello, I'm Haram.

Since posting this PR in June, I've been dealing with it so far.

Most checks passed, and once all checks passed. At that time, I reflected the request to erase the blank line, and an error occurred in the part I didn't deal with.

Right now, I'm just checking without adding or deleting the contents, but can the you please check my PR?

A total of 13 L2CAP commands have been added, and when this PR becomes merge, I will add commands from other Bluetooth protocols later. I'm really interested in Scapy, so I'll do more PR on scapy Bluetooth.

Thank you.

haramel avatar Nov 11 '21 04:11 haramel

Hi,

Any plans to merge this?

nrathaus avatar Jan 22 '23 06:01 nrathaus

@haramel this is a very nice PR in which you invested a lot of work and it is a pity that you had to struggle so much with the CI failures. I hope you haven't abandoned this PR yet and I am offering to help you with technical difficulties. Here just a few hints to get you started.

In my experience as a contributor, it is worth the effort to get the tests running locally. But before the test suite, the most elementary test is to start scapy interactively and see whether it loads your code and whether it works as expected:

~/src/scapy$ ./run_scapy 

In your case, the module didn't even load. After fixing those erros, you could run the bluetooth testsuite as follows:

~/src/scapy$ test/run_tests  -t test/scapy/layers/bluetooth.uts -F

You can run the entire test suite by running tox without arguments (you might need to install it first), however, it runs a lot of tests which are mostly unrelated to your changes and some of them require root privileges.

tox

I never run the entire test suite myself, primarily just selected tests which are related to my changes using run_tests. However, I recommend to run at least the mypy and flake8 tests locally which will point out common programming style and whitespace issues to you:

tox -e mypy -e flake8

And one final tip: not all CI failures are necessarily caused by your PR: the CI runs on a temporary merge commit created from merging your pull request branch with the master branch. So might be that the CI is failing on master already (which shouldn't happen in theory but does happen in practice). Whenever you are in doubt, you can take a look at the history of the master branch whether the HEAD commit has a red marker. Click on it, to see more details about the CI run on master.

grafik

In such cases, you can simply ignore it or write something like "CI failure xxx is unrelated" to indicate that you checked the failures.

mspncp avatar Jan 22 '23 14:01 mspncp

I rebased your branch on master and squashed your fixups. Then I tried to run scapy, but the module did not even load, because 'initial credits' contained a space:

~/src/scapy$ ./run_scapy 
INFO: Can't import PyX. Won't be able to use psdump() or pdfdump().
ERROR: Loading module scapy.layers.bluetooth
Traceback (most recent call last):
  File "/home/msp/src/scapy/bluetooth/scapy/main.py", line 154, in _load
    mod = importlib.import_module(module)
  File "/usr/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/home/msp/src/scapy/bluetooth/scapy/layers/bluetooth.py", line 484, in <module>
    class L2CAP_LE_Credit_Based_Connection_Request(Packet):
  File "/home/msp/src/scapy/bluetooth/scapy/base_classes.py", line 338, in __new__
    ] + [
  File "/home/msp/src/scapy/bluetooth/scapy/base_classes.py", line 339, in <listcomp>
    inspect.Parameter(f.name,
  File "/usr/lib/python3.10/inspect.py", line 2672, in __init__
    raise ValueError('{!r} is not a valid parameter name'.format(name))
ValueError: 'initial credits' is not a valid parameter name

...

After fixing the problem, the bluetooth test succeeded:

msp@debian:~/src/scapy/bluetooth$ test/run_tests  -t test/scapy/layers/bluetooth.uts -F
━ UTScapy - Scapy unknown.version - 3.10.9
 └ Non-root mode
 └ Booting scapy...
 └ Discovering tests files...
━ Loading: test/scapy/layers/bluetooth.uts
passed D839BB51 000.03s HCI layers
passed 34B7B7FD 000.00s Test HCI_PHDR_Hdr
passed 4CF7049E 000.00s LE Create Connection
passed 8D103942 000.01s LE Create Connection Cancel
passed D4F061CB 000.00s Disconnect
passed D9305D66 000.00s LE Connection Update Command
passed 6B099A56 000.00s LE Connection Update Event
passed E279ABBA 000.00s Parse EIR_Flags, EIR_CompleteList16BitServiceUUIDs, EIR_CompleteLocalName and EIR_TX_Power_Level
passed CBCEBE25 000.00s Parse EIR_Manufacturer_Specific_Data
passed 75E9A69D 000.01s Parse EIR_Manufacturer_Specific_Data with magic
passed D5066B69 000.01s Parse EIR_ServiceData16BitUUID
passed FAD81431 000.00s Basic L2CAP dissect
passed 88BBF466 000.01s Basic HCI_ACL_Hdr build & dissect
passed E06C4A06 000.00s Complex HCI - L2CAP build
passed 30734F0C 000.00s Complex HCI - L2CAP dissect
passed A86E9865 000.00s Answers
passed 6A7A605D 000.01s EIR_Hdr - HCI_LE_Meta_Advertising_Report (single report)
passed FE8DE930 000.02s EIR_Hdr - HCI_LE_Meta_Advertising_Report (duplicate reports)
passed 8AE37F5F 000.01s ATT_Hdr - misc
passed 15EAADC4 000.00s ATT Read_By_Type_Response
passed 9F072AC7 000.03s L2CAP layers
passed 1B61265D 000.00s SM_Public_Key() tests
passed 3B7175FE 000.00s SM_DHKey_Check() tests
Campaign CRC=98EB1E0C in 000.17s SHA=45790B2B3FE0F977C5EEFA98CF77029B537DBF5E
PASSED=23 FAILED=0
✓ All campaigns executed. Writing output...

You can find the repaired Pull Request with my fixes (as separate fixup!commit) here: https://github.com/mspncp/scapy/commits/haramel . Feel free to pick it up.

mspncp avatar Jan 22 '23 14:01 mspncp

@nrathaus Over time, I forgot this. But I want to merge this.

@mspncp Thank you. I revised the "initial credit".

haramel avatar Jan 23 '23 17:01 haramel