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

tlsfuzzer/extract.py: Add support for {r,s} tuples in sigs file #925

Merged
merged 1 commit into from
May 7, 2024

Conversation

GeorgePantelakis
Copy link
Contributor

@GeorgePantelakis GeorgePantelakis commented May 6, 2024

Checklist


This change is Reviewable

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

so, that's generally called raw encoding, see https://ecdsa.readthedocs.io/en/latest/basics.html#signature-formats

I wonder if instead of adding a "r_and_s", we shouldn't add a generic input_format parameter and allow it to have either "DER" (the default), or "raw", for the new option.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @GeorgePantelakis)


tlsfuzzer/extract.py line 82 at r1 (raw file):

    print(" --raw-sigs FILE Read the signatures from an external file.")
    print("                The file must be in binary format.")
    print(" --r-and-s      Specifies that the signatures in the binary file")

again, probably should use raw format


tlsfuzzer/extract.py line 920 at r1 (raw file):

        """Iterator. Read the signatures from file provided"""
        with open(filename if filename else self.sigs, "rb") as sigs_fp:
            if self.r_or_s_size:

instead of increasing indentation level, why not create two new private methods, and call them from here based on the expected signature format?


tlsfuzzer/extract.py line 932 at r1 (raw file):

                sig = sigs_fp.read(1)
                while sig:
                    if not ecdsa.der.is_sequence(sig):  # pragma: no cover

no, that's a branch that we ought to have test coverage for


tlsfuzzer/extract.py line 939 at r1 (raw file):

                    try:
                        sig_length = ecdsa.der.read_length(length_bytes)[0]
                    except ecdsa.UnexpectedDER:  # pragma: no cover

same here


tlsfuzzer/extract.py line 986 at r1 (raw file):

        if self.r_or_s_size:
            r_bytes = sig[:self.r_or_s_size]
            s_bytes = sig[self.r_or_s_size:]

why not use ecdsa.util.sigdecode_string: https://ecdsa.readthedocs.io/en/latest/ecdsa.util.html#ecdsa.util.sigdecode_string ?

@tomato42
Copy link
Member

tomato42 commented May 7, 2024

thinking a bit more: the command line option can also be a --sig-format [DER|RAW] instead of a flag, that will be more future proof

Copy link
Contributor Author

@GeorgePantelakis GeorgePantelakis left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tomato42)


tlsfuzzer/extract.py line 932 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

no, that's a branch that we ought to have test coverage for

Can you elaborate a bit more on why this (and next) path needs to have coverage? I mean it is not tlsfuzzer code and in general, it is a very simple misformating error, perhaps I am missing something.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @GeorgePantelakis)


tlsfuzzer/extract.py line 932 at r1 (raw file):

Previously, GeorgePantelakis (George Pantelakis) wrote…

Can you elaborate a bit more on why this (and next) path needs to have coverage? I mean it is not tlsfuzzer code and in general, it is a very simple misformating error, perhaps I am missing something.

This error will be raised if the user passes in wrong signature file (i.e. in raw format when the system is configured for DER), or the signature file is damaged.

i.e. it's a user facing check and error, therefore we should have test coverage that this kind of mistake is detected and reported appropriately

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @GeorgePantelakis)

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @GeorgePantelakis)

@tomato42 tomato42 merged commit 6b20975 into tlsfuzzer:master May 7, 2024
45 of 72 checks passed
@tomato42
Copy link
Member

tomato42 commented May 7, 2024

looks good, thanks!

@GeorgePantelakis GeorgePantelakis deleted the r_and_s_extract branch May 14, 2024 09:41
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.

None yet

2 participants