Refactoring + Insert Modification for Strings, Integer, BigInteger, Long + Some more Modifications for Long #182

C0D3D3V · 2024-11-04T16:23:43Z

⚠️ Warning: Downward compatibility is no longer given with this pull request. Old serialized modifiable variables with modifications can no longer be deserialized without changes to the XML.

Adds the possibility to add multiple modifications to a Modifiable Variable.

Includes some refactoring:

To make the switch cases more readable
To simplify some statements / conditions
To make linter happy

Adds some more modifications, including StingInsertValueModification

Modification classes. Also simplify some conditions in validateAssertions() and modify() methods.

…logging methods with parameterized log messages

…ification was not modified, just copied.

…ually return modified copies and not only the same modification.

…modification types instead of constant integers. This also unifies the method in all factories. Also remove unnecessary semicolons from previous commit.

…rayInsertModification class. To make the String modifiable variable more complete. Naming it InsertValue to keep it consistent with append and prepend, though the other Modification classes put "value" only behind Explicit Modifications.

* origin/main: build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.2.1 to 4.2.4 ci: Use Maven 3.9.9 ci: Switch to recordCoverage plugin build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.2.0 to 4.2.1 build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.1.17 to 4.2.0 build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.1.16 to 4.1.17 build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.1.10 to 4.1.16 build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.1.9 to 4.1.10 release: Prepare for next development iteration release: v4.2.2 Added suppressing adapters to leave default values out of XML build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.1.7 to 4.1.9 build(deps): bump de.rub.nds:protocol-toolkit-bom from 4.1.6 to 4.1.7 fixed and supressed warning added generics added supression of warning as its a test with reflections closes scanner after use

ic0ns

Rest looks pretty good

src/main/java/de/rub/nds/modifiablevariable/string/StringAppendValueModification.java

…it more semantic. There is no real reason for this change. Now the magic numbers, are a little bit less magic (just counting up the different modifications)

SSH-Fuzzer hat defined some modificiations using the explicitValueModificationGenerator() method. It makes not much sense for me to define them not well structured in the SSH-Fuzzer (and abusing explicit modifications for this). The Mutations that got moved over are: - Append - Insert - Prepend for Integer, BigInteger, ByteArray and Long These Modifications do currently produce no usefull values for negative parameters, but if someone needs this, he should revise this. For our work, we mostly need only positiv values. I also added the missing modifications for long, since SFTP uses longs. And also added multiply modifiaction to integer.

C0D3D3V · 2024-11-21T08:44:04Z

I have also changed the behavior of the insert modification of byteArray to adopt the behavioir of the new insert modifications, namely if the insert position is outside the possible, it behaves like an append or prepend instead of outputting the original value. Except that it still wraps around like before, as long as the negative position is greater than -2*input.length. Maybe I will add the wrap around to the other insert modifications too.

That got me thinking, I think I'll change the behavior completely, so that the insert works for arbitrary positions, which would be better for random positions that don't necessarily take the length of the original value into account.

…eger and Long, so that they actually make sense. Before the calculations where copied form SSH-Fuzzer, but they do not really produce values that makes any sense. Because the values were shifted and then a new value were added, but adding a value to a binary integer, does not produce the expected insertion. Now we insert the values bitwise, I see a use-case for this. e.g. in mask fields. Also let the insert position wrap around in all insert modifications. For int and long it wraps around the bit size.

…ons and use standard prime number instead.

Also some refactoring

…ng null as object. I think it makes no sense to allow null as copy object.

… wil see the same Warnings, in IntelliJ.

…icit value modifications.

See tls-attacker#182 (comment)

… the name pattern for the new eigthBytesToLong and fourBytesToInt methods.

C0D3D3V · 2025-02-24T15:16:44Z

To summarize, some wanted changes:

remove all ExplicitValueFromFile modifications
remove ModifiablePath with all of its modifications
remove all Modification Factories

I will only do this, as soon as I added a way to dynamically load xml descriptions of Modifications, else all my stored xml vectors would need major changes or I would need to use my own version of modifiable variables

ic0ns · 2025-02-25T07:08:44Z

I actually see it the other way around: I would have liked to have added the Adaptive Modification Generators from the Fuzzer project to this framework as well. But you do not like it, so I can remove the factories and the generation of random changes.

I can also move the not wanted Modifications to my fuzzer.

It's fine to have a separate package with functions and classes that create modifications - but the logic should not be within the modifiable variables. ModifiabeVariables do not have the context to make good choices for modifications. The code that was there for this purpose was basically there since the very beginning of the project when we were still experimenting with fuzzing designs... Like - if you have a modifiableInteger - a create randomModification call does not know that values that can actually be encoded in a message may only be within a certain range. This knowledge must be injected context specific from the outside.

ic0ns · 2025-02-25T07:10:55Z

To summarize, some wanted changes:
* remove all ExplicitValueFromFile modifications

* remove ModifiablePath with all of its modifications

* remove all Modification Factories
I will only do this, as soon as I added a way to dynamically load xml descriptions of Modifications, else all my stored xml vectors would need major changes or I would need to use my own version of modifiable variables

I am not sure what you mean with descriptions of modifications. If this is for a thesis, you must not switch to the newest version in the master branch and can stay at your own branch with your changes till the end.

Also removing all propOrder annotations, because without modificationFilter the order is the same as specified.

…cker#182 (comment) Projects that relayed on strong typing need now to add all Modifications and Mod Vars to the JAXB Context.

Move all Append, Insert, Prepend Modifications for Integer, Long and BigInteger to Fuzzer code, because it does not fit into the vision of ModifiableVariables Framework (and are not self explained because they work at binary level). Remove all Explicit Value from file modifications.

…lue is null. I used an extra message so one can distinguish it from "null" string.

C0D3D3V · 2025-02-26T16:06:39Z

ready for another review.

src/main/java/de/rub/nds/modifiablevariable/ModifiableVariable.java

src/main/java/de/rub/nds/modifiablevariable/string/ModifiableString.java

ic0ns · 2025-02-28T07:38:53Z

Other than that it looks good now can we can merge it after

…This makes it slower, but at least it is more similar to the rest of the attacker ecosystem. Remove getFirstModification() because you can use getModifications()

Add clearModifications() because a call with null on setModifications() is not possible.

C0D3D3V · 2025-02-28T09:10:42Z

I added a clearModifications() method. If you do not want it, I can remove it. You could archive the same when calling setModifications() without argument.

C0D3D3V · 2025-02-28T09:24:11Z

In last commit I replaces all setModification calls with setModifications calls. I could also use the new addModification method, that would avoid the overhead, in the Modifiable class, not in the tests.

C0D3D3V · 2025-02-28T13:18:25Z

I just noticed that all modifications are now named differently. They all start with a lower case letter. For example, ByteArrayInsertValueModification is now named byteArrayInsertValueModification. Previously, the name was set explicitly. Should I still change that? Or do you not care? 02b7eaa#diff-9e2c450cee5dc51a7658b65a833e95b3aaa7fae666e7d27121e96cf38c2045d7L37

-> All old serialized modifications cannot be loaded without modification. (but they do not load anyway because they are not in a <modifications> tag that now is required.)

My opinion is that names should be lowerCamelCase. It looks more natural, and most XML schemas I'm familiar with use it. Furthermore, hard-coding names per class could lead to typos or unclear schemas, or forgetting to specify them. For example, TLS-Attacker uses lowerCamelCase for some names and upperCamelCase for some others. I think it would be best to stick with JAXB default naming as much as possible.

C0D3D3V added 8 commits October 12, 2024 16:13

Simplify all return statements in the equal() methods of the

a1cb9cc

Modification classes. Also simplify some conditions in validateAssertions() and modify() methods.

Replace constant string concatenations that are used as arguments to …

7f3fb2f

…logging methods with parameterized log messages

Fix getModifiedCopy() of ByteArrayShuffleModification. Before the Mod…

4ee5dd0

…ification was not modified, just copied.

Fix getModifiedCopy() of StringModifications, so that the methods act…

24750a5

…ually return modified copies and not only the same modification.

Make createRandomModification() more readable by using Enums for the …

31bfb2f

…modification types instead of constant integers. This also unifies the method in all factories. Also remove unnecessary semicolons from previous commit.

Fix Typo getinsertValue to getInsertValue

457a67b

ic0ns reviewed Nov 4, 2024

View reviewed changes

src/main/java/de/rub/nds/modifiablevariable/string/StringAppendValueModification.java Outdated Show resolved Hide resolved

ic0ns previously approved these changes Nov 5, 2024

View reviewed changes

ic0ns enabled auto-merge November 5, 2024 18:34

ic0ns and others added 4 commits November 6, 2024 14:32

Merge branch 'main' into daniel-main

79c0218

fix bytesToLong

fc9d71b

Change the hashCode() function of all Modifications, so they have a b…

8599859

…it more semantic. There is no real reason for this change. Now the magic numbers, are a little bit less magic (just counting up the different modifications)

auto-merge was automatically disabled November 21, 2024 08:36
Head branch was pushed to by a user without write access

C0D3D3V dismissed ic0ns’s stale review via 9789ee7 November 21, 2024 08:36

C0D3D3V added 3 commits November 21, 2024 10:04

fix tests

413d6d4

run formatter

3337cba

C0D3D3V changed the title ~~Refactoring + Insert Modification for Strings~~ Refactoring + Insert Modification for Strings, Integer, BigInteger, Long + Some more Modifications for Long Nov 21, 2024

C0D3D3V added 8 commits November 21, 2024 11:34

Add simple insertion test for long and BigInteger

0e71c4c

Actually remove magic values from hashCode() method of all modificati…

8c67bff

…ons and use standard prime number instead.

Add copy constructors to all modifiable variables

5f31e08

Also some refactoring

Fix equals() of ModifiableLengthField (I broke it in last commit)

9598313

Undo changes to use String.format() instead of string concatenation

580b528

Change CopyConstructors. Throw NullPointerException instead of allowi…

b153a9e

…ng null as object. I think it makes no sense to allow null as copy object.

Add Project_Defaults.xml to repo. So everyone developing on this repo…

4b606ee

… wil see the same Warnings, in IntelliJ.

Add FromFileModifications, so one can distinguish it from normal expl…

509333d

…icit value modifications.

C0D3D3V added 4 commits February 24, 2025 15:04

Remove getModifiedCopy()

3f8d6c3

See tls-attacker#182 (comment)

Remove now unused limit constants, because I removed getModifiedCopy

01ecd03

Remove BigIntegerInteractiveModification

840f698

See tls-attacker#182 (comment)

Revert names of ArrayConverter methods back to the old one. And adapt…

922914a

… the name pattern for the new eigthBytesToLong and fourBytesToInt methods.

C0D3D3V added 10 commits February 26, 2025 08:54

Remove inspection profile, because of tls-attacker#182 (comment)

51401fd

remove reduceToOriginalValue, see tls-attacker#182 (comment)

d75be13

Remove modification filter feature see: tls-attacker#182 (comment)

deb69e2

Also removing all propOrder annotations, because without modificationFilter the order is the same as specified.

Use Pattern Variables where possible

3c3b6a0

Remove softlySet Methods. See tls-attacker#182 (comment)

7fa7448

Use @XmlAnyElement(lax = true) instead of strong typing. See tls-atta…

02b7eaa

…cker#182 (comment) Projects that relayed on strong typing need now to add all Modifications and Mod Vars to the JAXB Context.

Remove explicit value from file tests I forgot to delete

f3957ea

Fix debug method in VariableModification for cases where resulting va…

012c0cf

…lue is null. I used an extra message so one can distinguish it from "null" string.

Move String valueString definition into the case.

e9ba2b5

ic0ns reviewed Feb 28, 2025

View reviewed changes

Merge branch 'main' into daniel-main

ea4fac4

C0D3D3V added 2 commits February 28, 2025 09:48

Remove setModification(), because you can use setModifications(...). …

449ca21

…This makes it slower, but at least it is more similar to the rest of the attacker ecosystem. Remove getFirstModification() because you can use getModifications()

Replace all setModification() calls with setModifications()

3567f7b

Add clearModifications() because a call with null on setModifications() is not possible.

ic0ns approved these changes Feb 28, 2025

View reviewed changes

ic0ns enabled auto-merge February 28, 2025 09:31

ic0ns disabled auto-merge March 4, 2025 08:14

ic0ns merged commit 7678c5b into tls-attacker:main Mar 4, 2025
1 check passed

Refactoring + Insert Modification for Strings, Integer, BigInteger, Long + Some more Modifications for Long #182

Refactoring + Insert Modification for Strings, Integer, BigInteger, Long + Some more Modifications for Long #182

Uh oh!

Conversation

C0D3D3V commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ic0ns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

C0D3D3V commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

C0D3D3V commented Feb 24, 2025

Uh oh!

ic0ns commented Feb 25, 2025

Uh oh!

ic0ns commented Feb 25, 2025

Uh oh!

C0D3D3V commented Feb 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ic0ns commented Feb 28, 2025

Uh oh!

C0D3D3V commented Feb 28, 2025

Uh oh!

C0D3D3V commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

C0D3D3V commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

C0D3D3V commented Nov 4, 2024 •

edited

Loading

C0D3D3V commented Nov 21, 2024 •

edited

Loading

C0D3D3V commented Feb 28, 2025 •

edited

Loading

C0D3D3V commented Feb 28, 2025 •

edited

Loading