ModSecurity icon indicating copy to clipboard operation
ModSecurity copied to clipboard

Update libinjection & Mbed TLS

Open eduar-hte opened this issue 1 year ago • 4 comments

what

Update third-party dependencies (included in others directory): libinjection & Mbed TLS.

why

The versions included in ModSecurity have not been updated in a while:

  • libinjection currently references commit libinjection/libinjection@bfba51f from Dec, 2020.
    • This PR references the current head of the library (libinjection/libinjection@b9fcaaf), as the project isn't currently being tagged (the last one is from 2017).
    • Even though the project hasn't move a lot since then, there are at least a couple of changes that may be interesting:
      • libinjection/libinjection@a28aceec29932d216f487b09882667479de717ed
      • libinjection/libinjection@98cf343b0c94b7e9a94aadcaa94ef779f7e16633
  • A subset of Mbed TLS source code from (at least) Nov, 2016 is included in the ModSecurity repository.
    • This PR references the latest tag of the library (3.6.0, Mbed-TLS/mbedtls@2ca6c28) and instead of including their source code, uses a submodule (which makes it clear the version being used and simpler to update in the future).
    • A few interesting changes in the referenced subset of the library (base64, md5 & sha1) since the last update (all of them may not be relevant in ModSecurity usage):
      • Mbed-TLS/mbedtls@ab043350525833314b5a2e6d0d2efcd6510a8142
      • Mbed-TLS/mbedtls@1cc1fb05999aea8067e11f5c4f4fdb32dbe91036
      • Mbed-TLS/mbedtls@d1c98fcf5e04b8f6628ca951131aa1f7d67283de
      • Mbed-TLS/mbedtls@0963e6cfac2230d68c6ed1aa220ac41f096796ff

eduar-hte avatar May 31 '24 01:05 eduar-hte

We should indeed update these libraries from time to time. This PR dynamically uses the last version (master), right? It seems they don't provide a release version. Do you (and other people contributing to this repo, I some some other ones) consider the master version as always stable? Btw, the same should be done for v2 as well.

marcstern avatar May 31 '24 08:05 marcstern

This PR dynamically uses the last version (master), right? It seems they don't provide a release version. Do you (and other people contributing to this repo, I some some other ones) consider the master version as always stable?

In both cases the PR references a specific commit hash in order to pin down the version included in libModSecurity until a new review and update is done.

With regards to libinjection, as the project is not currently tagged (the last tag is from 2017), I'm referencing the current head (libinjection/libinjection@b9fcaaf). Mbed TLS references the commit for tag 3.6.0 (git submodules don't currently support explicitly referencing a tag), Mbed-TLS/mbedtls@2ca6c28.

eduar-hte avatar May 31 '24 13:05 eduar-hte

Hi @eduar-hte,

thanks again for this great PR!

Fortunately @fzipi is one of the maintainer/developer of libinjection project so I tagged him - may be he can help us.

airween avatar May 31 '24 19:05 airween

Thanks @eduar-hte for this great work again.

A reminder to myself: add git submodule init and git submodule update commands to README.md.

airween avatar Jul 10 '24 14:07 airween

What you normally do is use the checkout actions like this:

      - name: Checkout
        uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4
        with:
          submodules: recursive

That will bring you what you want.

fzipi avatar Jul 10 '24 14:07 fzipi

So, we should replace the hard-coded files in v2 with submodules as well, right?

marcstern avatar Jul 11 '24 08:07 marcstern

What you normally do is use the checkout actions like this: (...)

That's right, the submodules option simplifies checkout of the repository and also its submodules. However, I found out that the git describe command would not work when the submodules are initialized that way, so I replaced that (arguably simpler approach) with an additional step to do those steps manually as a workaround.

If you take a look at the build with the workaround, you'll see that the 'configure' step shows version info on MbedTLS & libInjection correctly:

 Mandatory dependencies
   + libInjection                                  ....v3.9.2-92-gb9fcaaf
   + Mbed TLS                                      ....v3.6.0
   + SecLang tests                                 ....a3d4405

However, if you look at an older build, you'll notice that the version information is missing:

 Mandatory dependencies
   + libInjection                                  ....
   + SecLang tests                                 ....a3d4405

eduar-hte avatar Jul 17 '24 01:07 eduar-hte

Oh, for describe you might also want to use depth: 0

fzipi avatar Jul 17 '24 09:07 fzipi

Oh, for describe you might also want to use depth: 0

You're right, fetch-depth: 0 works! (I thought I had tested that before going for the workaround 🤷‍♂️)

I just created a simple PR (#3185) to simplify the workflow again, thanks.

eduar-hte avatar Jul 17 '24 23:07 eduar-hte