Conversation
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
|
Thanks for this impressive PR. I am unwilling to merge this in without also having an end-to-end client + server example for it, similar to this for 0x27: https://github.com/driftregion/iso14229/tree/683180a7765d19f9e08be00de2517a9f095b9b5d/examples/linux_server_0x27. The reasoning is: this library aims to make a decent UDS implementation available to anyone with minimal cognitive overhead. I think 0x29 as described in the standard is too complex. (That said, It looks like you've done a fine job implementing it!) The role of this library is to provide good defaults -- to make it easy for the integrator to copy and paste this library (or our example code) in and have it work in a predictable and secure way. My expectation is that the integrator need not have an in-depth knowledge of the standard to do this. |
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Thanks for the quick feedback! 👍 I added a simple example. I also noticed that the other samples seem to be out of date (e.g. returning But to ensure the samples at least compile, I suggest adding a quick CI job that builds the (linux-) samples. |
f037fc2 to
ac6d797
Compare
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
|
Hi @ParaZera. Thanks for adding the example. It runs great. I had to patch the Makefile as follows to work with the 2.28.0-1build1 libmbedtls-dev installed on my machine. diff --git a/examples/linux_server_0x29/Makefile b/examples/linux_server_0x29/Makefile
index 3bc31930..de98718d 100644
--- a/examples/linux_server_0x29/Makefile
+++ b/examples/linux_server_0x29/Makefile
@@ -6,10 +6,10 @@ TARGETS = server client
PKGS = mbedtls mbedx509 mbedcrypto
CFLAGS = -DUDS_TP_ISOTP_SOCK -DUDS_LINES -g -I ../..
-CFLAGS += $(shell pkg-config --cflags $(PKGS))
+CFLAGS += $(shell pkg-config --cflags $(PKGS) 2>/dev/null)
# Use LDLIBS for libraries; order matters (crypto last)
-LDLIBS = $(shell pkg-config --libs $(PKGS))
+LDLIBS = $(shell pkg-config --libs $(PKGS) 2>/dev/null || echo "-lmbedtls -lmbedx509 -lmbedcrypto")
# AES key artifacts
KEY_BIN = uds_aes_key.binMore importantly, please run the fuzzer. I am using the following command: # Get workspace path
WORKSPACE=$(bazel info workspace)
bazel run -c opt --config=hermetic-fuzz //fuzz:fuzz_server_bin -- \
-max_total_time=60 \
-artifact_prefix=${WORKSPACE}/fuzz/outputs/ \
-jobs=64 \
${WORKSPACE}/fuzz/outputs/corpusI am finding many stack use after return crashes. Please investigate these. |
Thanks. This is fixed in #105 |
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
I applied the changes as they also work on my machine.
I was running the fuzzer and getting one or two crashes, but when replaying them, they seemed to work, so I thought they were false positives. Now I know what I did wrong. I had to make multiple runs for the error to occur again, e.g. with: BIN="$(bazel info --config=hermetic-fuzz --compilation_mode=dbg bazel-bin)/fuzz/fuzz_server_bin"
"$BIN" -runs=1000 -error_exitcode=101 "${PWD}/fuzz/outputs/crash-9c0ac94c5819ce8897516b77de59f203ec04be1f"It's the first time for me working with fuzzing (and bazel), so sorry in advance for any further newbie mistakes I may make. Do you want me to add the crashes to the corpus in future PR's? (I already deleted the ones I generated for this PR) |
Great, thanks
I saw the same behavior. Crashes were not reproducible.
No worries. I started learning about fuzzing recently too. It seems like there may be two issues here:
If the fuzzing target is deterministic, the same input should result in the same output (or crash) every time.
No. The corpus is the set of inputs which exercise unique paths through the library code. A crash means that there exists an input which crashes the library. A contract I seek to uphold is that the server should never crash as a result of any request data. Fuzzing is an imperfect but powerful way of enforcing this contract. The fuzz target (fuzz:fuzz_server) is compiled with asan (enabled via clang flag -fsanitize=address) which checks for memory errors (e.g. array out-of-bounds) at runtime. I view the coverage report like this: sudo apt install lcov
scripts/1_run_fuzzer.sh
scripts/2_replay_corpus.sh
genhtml --function-coverage --branch-coverage --output-directory coverage_html coverage.lcov
xdg-open coverage_html/index.html Studying this coverage report while asking the question: "why was this line not reached?" yields valuable insight into things like inaccessible branches, ways to collapse the logic and simplify the code, as well as limitations in the harness ( Please view the fuzz coverage of the newly added handler and find ways to increase it. |
|
Sorry for the delayed response. FYI: Currently there are other projects that take my focus. It might take some time until I can look into the fuzzy tests. |
Signed-off-by: Tim Schrader <tim.schrader@frickly.systems>
|
No worries. Thanks for the great work on this so far. We'll get this merged sooner or later. |
|
I had a quick look at it and the problem with the many missed branches seems to be the following: Since we bombard the server with just random bytes, it is very improbable that the input survives the validity checks I do while parsing the request. E.g. the One way I found that could help would be using custom mutators to do structure aware fuzzing. As far as I understood, it would mean splitting the fuzz server into two (or more) separate binaries (and different corpus etc.) that can generate more fitting input for each individual request to better match the required structure. Is that something you would want in your project? The other way I found is to use another fuzzer that has a predefined grammar. |
|
On another note, when I generate the coverage I get many inconsistency errors and have to use ./scripts/2_replay_corpus.sh
+ bazel coverage -c opt --config=hermetic-coverage //fuzz:fuzz_server
INFO: Using default value for --instrumentation_filter: "^//fuzz[/:]".
INFO: Override the above default with --instrumentation_filter
INFO: Analyzed target //fuzz:fuzz_server (1 packages loaded, 30006 targets configured).
INFO: LCOV coverage report is located at /home/tim/.cache/bazel/_bazel_tim/5b3b06866e5cdfb3d4d9ddbeafb79e46/execroot/_main/bazel-out/_coverage/_coverage_report.dat
and execpath is bazel-out/_coverage/_coverage_report.dat
INFO: Found 1 test target...
Target //fuzz:fuzz_server up-to-date:
bazel-bin/fuzz/fuzz_server
INFO: Elapsed time: 6.079s, Critical Path: 5.39s
INFO: 4 processes: 17 action cache hit, 4 linux-sandbox.
INFO: Build completed successfully, 4 total actions
//fuzz:fuzz_server PASSED in 3.4s
/home/tim/.cache/bazel/_bazel_tim/5b3b06866e5cdfb3d4d9ddbeafb79e46/execroot/_main/bazel-out/k8-opt/testlogs/fuzz/fuzz_server/coverage.dat
Executed 1 out of 1 test: 1 test passes.
++ bazel info output_path
+ cp -f /home/tim/.cache/bazel/_bazel_tim/5b3b06866e5cdfb3d4d9ddbeafb79e46/execroot/_main/bazel-out/_coverage/_coverage_report.dat coverage.lcov
+ sed -E -i 's#^SF:bazel-out/.*/iso14229.c#SF:iso14229.c#' coverage.lcov
+ sed -E -i 's#^SF:bazel-out/.*/iso14229.h#SF:iso14229.h#' coverage.lcov
+ ls -l coverage.lcov
-r-xr-xr-x 1 tim tim 67983 Oct 14 11:05 coverage.lcov
genhtml --function-coverage --branch-coverage --ignore-errors inconsistent --output-directory coverage_html coverage.lcov
Reading tracefile coverage.lcov.
genhtml: WARNING: (unsupported) Function begin/end line exclusions not supported with this version of GCC/gcov; require gcc/9 or newer: attempting to derive function end lines - see lcovrc man entry for 'derive_function_end_line'.
(use "genhtml --ignore-errors unsupported,unsupported ..." to suppress this warning)
genhtml: WARNING: (inconsistent) "fuzz/fuzz_server.cc":104: line is not hit but at least one branch on line has been evaluated.
To skip consistency checks, see the 'check_data_consistency' section in man lcovrc(5).
(use "genhtml --ignore-errors inconsistent,inconsistent ..." to suppress this warning)
genhtml: WARNING: (inconsistent) "iso14229.c":1273: line is not hit but at least one branch on line has been evaluated.
genhtml: WARNING: (inconsistent) "iso14229.c":1986: line is not hit but at least one branch on line has been evaluated.
genhtml: WARNING: (inconsistent) "iso14229.c":1213: line is hit but no branches on line have been evaluated.
genhtml: WARNING: (inconsistent) "iso14229.c":2676: line is hit but no branches on line have been evaluated.
{ ... }
genhtml: WARNING: (inconsistent) "iso14229.c":2632: line is not hit but at least one branch on line has been evaluated.
genhtml: WARNING: (inconsistent) "iso14229.c":1269: line is hit but no branches on line have been evaluated.
genhtml: WARNING: (inconsistent) "iso14229.c":1937: line is not hit but at least one branch on line has been evaluated.
genhtml: WARNING: (inconsistent) "iso14229.c":1282: line is hit but no branches on line have been evaluated.
genhtml: WARNING: (inconsistent) "iso14229.c":1262: line is hit but no branches on line have been evaluated.
Found 3 entries.
Found common filename prefix "/home/tim/frickly/projects/mercedes/ardep-workspace"
Generating output.
Processing file iso14229.c
lines=2729 hit=1522 functions=81 hit=46 branches=1438 hit=710
Processing file fuzz/fuzz_server.cc
lines=315 hit=302 functions=7 hit=7 branches=104 hit=101
Processing file iso14229.h
lines=103 hit=94 functions=6 hit=3
Overall coverage rate:
source files: 3
lines.......: 60.9% (1918 of 3147 lines)
functions...: 59.6% (56 of 94 functions)
branches....: 52.6% (811 of 1542 branches)
Message summary:
49 warning messages:
inconsistent: 48
unsupported: 1 |
47d1a3d to
25ca1cf
Compare
|
|
I've spend some more time trying to get fuzzied structured data and looked at LPM which sounded like a good way to generate the data. But as far as I can tell now, it still needs some major work to be done because I can't set fixed sizes for byte arrays and by default any field in a protobuf-v3 message can be null. This obviously makes it hard to generate input where every required field is set. Maybe there is a way to give the fuzzer enough of a seed corpus to circumvent this most of the time, but I couldn't get it to work. The current state of the wip branch ist here. I may be able to revisit this problem in a few months, but right now I won't be working on it anymore. I've updated the branch into a clean state so you can merge the authentication service. |



Added 0x29 Authentication Function to the server:
I opted not to keep any specific state in the
UDSServer_tinstance as it is user-specific to keep the implementation lightweight. I also opted against an additionalvoid*element in the server for user specific authentication data because these could just as easily reside invoid *fn_data. This means that the server does no authentication checking and it is left to the user to correctly manage the authentication. This is similar to the security access level, but with no state kept in the server directly.I also created a simple AuthTimeout event, that fires when:
As we don't have any state on the authentication, I thought about also leaving this up to the user to implement, but since we're talking about security and every user of the server must handle a timeout and session change anyway, I opted to create this event. It can safely be ignored by the user (by design) and is just there to give the user minimal baseline where de-authentication should be done by the server. If you want, I could add a config option to disable/remove the event, but as it is a simple event that can safely be ignored, I don't think that it is necessary.
Since I don't use the client, I created the client-side of the example using bare
UDSSendBytes(). When the client side is implemented, it should be easily replaceable.The update of the iso14229.c/h files are build with a slightly different bazel/gcc version.