Skip to content

Add Honeysap compatibility#93

Open
randomstr1ng wants to merge 7 commits into
OWASP:masterfrom
randomstr1ng:honeysap-compatibility
Open

Add Honeysap compatibility#93
randomstr1ng wants to merge 7 commits into
OWASP:masterfrom
randomstr1ng:honeysap-compatibility

Conversation

@randomstr1ng

Copy link
Copy Markdown
Collaborator

This PR add required function and modules to add full compatibility for the HoneySAP Python3 PR #11

In addtion, this PR expands the integration and support for the SAP RFC protocol.

@randomstr1ng

Copy link
Copy Markdown
Collaborator Author

@martingalloar this is the PR I mentioned ealier that builds the foundation for HoneySAP. If you do not find any issues we can merge this and start working on HoneySAP.

thank you!

Comment thread pysap/SAPNWRFC.py
if name_min <= name_len <= name_max and name_end <= len(raw):
name = decode_value(raw[idx + 6:name_end])
if name and _RFC_PARAM_NAME_RE.match(name):
val, _ = find_tlv_field_by_marker(raw, 0x0203, name_end)

@martingalloar martingalloar Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Went through this one a few times and I'm not sure I fully get it without having more context on SAPNWRFC. This might be fine for now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm working on future improvements on this at the moment especially on getting a proper RFC implementation and parsing in place. But as I said, this will be a future PR. Right now, this was "just to get HoneySAP" working.

Comment thread pysap/utils/fields.py
Comment thread pysap/SAPNWRFC.py
if raw is None:
return None

if len(raw) >= 4 and raw[1:2] == b"\x00" and raw[3:4] == b"\x00":

@martingalloar martingalloar Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The UTF-16LE detection here is fairly coarse. A non-ASCII UTF-16LE payload or a binary field with the same byte pattern can be decoded incorrectly. I'm not sure there's a more deterministic way to check for the right payload while doing the decoding.

@DaCodah

DaCodah commented Jul 2, 2026

Copy link
Copy Markdown

Hi @martingalloar, @randomstr1ng,

I picked up the review comment about missing roundtrip coverage for the encoding-aware branch in utils/fields.py and wrote two tests for StrFixedLenPaddedField with encoding="utf-16-le":

  1. A basic roundtrip: addfield must produce the exact UTF-16LE bytes with NUL padding to the fixed width (e.g. "AB" at length 8 becomes b"A\x00B\x00\x00\x00\x00\x00"), and getfield must return the original str with padding stripped.
  2. The same roundtrip with a non-ASCII character to pin the behavior UTF-16LE actually exists for.

Both pass locally on top of honeysap-compatibility, full suite is green (582 passed). The branch is here if useful: https://github.com/DaCodah/pysap/tree/test/utf16le-roundtrip

@randomstr1ng happy to open a small PR against your honeysap-compatibility branch so it flows into this PR, or feel free to cherry-pick the commit directly. Whatever is easiest for you.

@randomstr1ng

Copy link
Copy Markdown
Collaborator Author

Hi @martingalloar, @randomstr1ng,

I picked up the review comment about missing roundtrip coverage for the encoding-aware branch in utils/fields.py and wrote two tests for StrFixedLenPaddedField with encoding="utf-16-le":

  1. A basic roundtrip: addfield must produce the exact UTF-16LE bytes with NUL padding to the fixed width (e.g. "AB" at length 8 becomes b"A\x00B\x00\x00\x00\x00\x00"), and getfield must return the original str with padding stripped.
  2. The same roundtrip with a non-ASCII character to pin the behavior UTF-16LE actually exists for.

Both pass locally on top of honeysap-compatibility, full suite is green (582 passed). The branch is here if useful: https://github.com/DaCodah/pysap/tree/test/utf16le-roundtrip

@randomstr1ng happy to open a small PR against your honeysap-compatibility branch so it flows into this PR, or feel free to cherry-pick the commit directly. Whatever is easiest for you.

Hi @DaCodah - thank you for you suggestion. I think that would make perfect sense for this: #93 (comment)

if you can open a PR that would be great, then we can keep this single PR - thank you!

@DaCodah

DaCodah commented Jul 3, 2026

Copy link
Copy Markdown

Done! PR is open here: randomstr1ng/pysap-dev#
Thanks for the quick feedback!

Add UTF-16LE roundtrip tests for StrFixedLenPaddedField
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.

3 participants