NULL pointer dereference fixes

[PrivacyIsARight]

1. Fixed off-by-one errors in calloc and malloc calls by adding the required byte for the null terminator.

2. Added string length guards before strncmp operations to prevent potential buffer over-reads on short paths.

3. Hardened pointer arithmetic by ensuring strchr results are not NULL before incrementing them.

4. Replaced assert() with conditional checks to allow the tool to skip malformed entries gracefully instead of crashing.

5. Improved hashing safety by using sizeof on the destination buffer (xe->sha256) rather than local variables.

6. Ensured memory integrity by calling free() on local allocations before skipping iterations in the new error paths.

You can test the PoC out with a command like this.

ASAN_OPTIONS=detect_leaks=0 ./bin/xbps-create/xbps-create \
    -A noarch -n "malicious-pkg-1.0_1" \
    -s "PoC for NULL dereference" \
    --alternatives "MALFORMED_ENTRY_NO_COLON" \
    mock_pkg 2>&1 | tee xbps_asan_output.txt

[PrivacyIsARight]

With the changes applied it now returns:

ASAN_OPTIONS=detect_leaks=0 ./bin/xbps-create/xbps-create
-A noarch -n "malicious-pkg-1.0_1"
-s "PoC for NULL dereference"
--alternatives "MALFORMED_ENTRY_NO_COLON"
mock_pkg 2>&1 | tee xbps_asan_output.txt
xbps-create: WARNING: ignoring malformed alternative: MALFORMED_ENTRY_NO_COLON
malicious-pkg-1.0_1: adding `./usr/bin/malicious-bin' ...
malicious-pkg-1.0_1: binary package created successfully (malicious-pkg-1.0_1.noarch.xbps)

[Duncaen]

Sorry but the commit is umergable, this should be split up into separate commits for each independent change. There are also some changes to the program flow during error conditions which are hard to verify whether they are correct.

And please disclose whether you used any tools to assist with this.

[PrivacyIsARight]

Sorry but the commit is umergable, this should be split up into separate commits for each independent change. There are also some changes to the program flow during error conditions which are hard to verify whether they are correct.

And please disclose whether you used any tools to assist with this.

Aren't changes supposed to be squashed?

[Duncaen]

Aren't changes supposed to be squashed?

No, why should they.

[PrivacyIsARight]

Aren't changes supposed to be squashed?

No, why should they.

Oh, I was following from the advice you gave me on void packages repo.

For tools I used, ASAN, gcc, gdb, and some others.

[PrivacyIsARight]

I'll try and separate the commits now. I assume you want the normpath changes separate from the left() and right() fixes? @Duncaen

[Duncaen]

I'll try and separate the commits now. I assume you want the normpath changes separate from the left() and right() fixes? @Duncaen

I kinda want every single change separately. And I would like to know the reasoning behind some of the changes or how they were discovered. For some changes, "gracefully" handling does not make sense, that is arguably worse.

[PrivacyIsARight]

I'll try and separate the commits now. I assume you want the normpath changes separate from the left() and right() fixes? @Duncaen

I kinda want every single change separately. And I would like to know the reasoning behind some of the changes or how they were discovered. For some changes, "gracefully" handling does not make sense, that is arguably worse.

I assume this is because assert causes it to crash instantly which reveals the issue immediatly. Would die() be a good middleground here?

[Duncaen]

Yes, errors should be handled like errors, in context where die is used and safe this can be done. In other cases like the library, neither is really appropriate.

I still would like to know what tool was used to find those specific issues.

[PrivacyIsARight]

Yes, errors should be handled like errors, in context where die is used and safe this can be done. In other cases like the library, neither is really appropriate.

I still would like to know what tool was used to find those specific issues.

I used claude to identify the issue and then I used the manual tools like ASAN, gdb, etc to craft the poc.

[PrivacyIsARight]

Yes, errors should be handled like errors, in context where die is used and safe this can be done. In other cases like the library, neither is really appropriate.

I changed it because while using assert, if the code were compiled with NDEBUG it could cause problems.

[Duncaen]

Yes, errors should be handled like errors, in context where die is used and safe this can be done. In other cases like the library, neither is really appropriate.

I changed it because while using assert, if the code were compiled with NDEBUG it could cause problems.

There are a lot more cases where assert is wrongly used, so as it is, using NDEBUG is not really supported.

[PrivacyIsARight]

Yes, errors should be handled like errors, in context where die is used and safe this can be done. In other cases like the library, neither is really appropriate.

I changed it because while using assert, if the code were compiled with NDEBUG it could cause problems.

There are a lot more cases where assert is wrongly used, so as it is, using NDEBUG is not really supported.

If so, then this could be something you could add to the build instructions. If you want, I can change it back to using assert, and add the EINVAL back in. I just figured that it would be a good change.

[Duncaen]

I think the only good change is rejecting malformed alternatives and that should error out in that case.

[Duncaen]

And maybe the changes to strchr, but that requires more review due to error handling during package install.

The strcmp changes make already hard to follow code worse.

[PrivacyIsARight]

So revert to EINVAL and assert, and remove the strcmp changes? Is the change to if (!xbps_file_sha256(xe->sha256, sizeof xe->sha256, fpath)) still fine in this case?

[Duncaen]

That's fine, but separate from the other changes.

[PrivacyIsARight]

These should be the changes we are able to add without issue.