fix: numexpr to fix CVE-2023-39631⁠ (2.8.4 => 2.9.0)

[nigzak]

SUMMARY

superset 3.1.0 / 3.1.1 has a critical finding: CVE-2023-39631⁠ because of numexpr 2.8.4

TESTING INSTRUCTIONS

unknown (sorry), this pull request is to update the numexpr to a non-faulty version depending the CVE (2.9.0 is the current latest available version based on https://github.com/pydata/numexpr)
To test "my" finding: open final image in docker scout => no finding for numexpr is there anymore

ADDITIONAL INFORMATION

• Has associated issue: Update superset "numexpr 2.8.4" dependency in docker image of superset 3.1.0/3.1.1 because of CVE-2023-39631⁠ #26967
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

HINT: This is my first pull request ... if I made something wrong please let me know that I can make a next one better :)

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[nigzak]

Hint: made a "manual dirty" update in the docker image to update to 2.9.0 (which for sure kills starting the docker image).

from apache/superset:3.1.1

USER root:root

RUN pip uninstall numexpr --yes
RUN pip install numexpr==2.9.0


USER superset:superset

With this update there is no critical finding of the CVE-2023-39631⁠ in the container anymore - an update of numexp should finally fix this finding

[nigzak]

fixed name of PR (seems I forgot the ":" after chore)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.61%. Comparing base (ad6327d) to head (8634285).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27187      +/-   ##
==========================================
- Coverage   69.72%   69.61%   -0.12%     
==========================================
  Files        1908     1908              
  Lines       74550    74550              
  Branches     8317     8317              
==========================================
- Hits        51982    51896      -86     
- Misses      20515    20601      +86     
  Partials     2053     2053              

Flag	Coverage Δ
hive	?
mysql	77.97% <ø> (-0.03%)	⬇️
postgres	78.10% <ø> (ø)
presto	?
python	82.88% <ø> (-0.24%)	⬇️
sqlite	77.62% <ø> (ø)
unit	56.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[john-bodley]

@nigzak it seems like numexpr is an indirect dependency:

$ git grep numexpr
requirements/base.txt:numexpr==2.8.4
requirements/base.txt:    #   numexpr

To ensure that CVE is addressed for all situations (and not just CI etc. which leverage the pinned/frozen dependencies) we should add the necessary bump to setup.py. This should likely be done by updating the pandas constraints given that's the package which is depending on it.

[dpgaspar]

@nigzak it seems like numexpr is an indirect dependency:

$ git grep numexpr
requirements/base.txt:numexpr==2.8.4
requirements/base.txt:    #   numexpr

To ensure that CVE is addressed for all situations (and not just CI etc. which leverage the pinned/frozen dependencies) we should add the necessary bump to setup.py. This should likely be done by updating the pandas constraints given that's the package which is depending on it.

Adding the constraint to setup.py is more complete, yet when applied as a global rule for indirect dependencies can cause hard to fix and unexpected problems with our extra packages. Also as time passes we will probably end up with a huge setup.py with lot's of indirect dependency constraints that will be hard to manage.

My take here is that we should keep setup.py restricted to direct dependencies only, and open exceptions for breaking changes only. Then add these constraints on requirements/*.in.
Users that use pip install apache-superset will get the latest packages when possible, or manage their dependencies on their own requirements file(s). Our requirements will ensure CI and an up to date docker image(s)

That being said, I agree that this should be done on pandas, but they are still on 2.8.4: https://github.com/pandas-dev/pandas/blob/main/pyproject.toml#L103

[nigzak]

HI @dpgaspar
that is bad to me, I am not allowed to deploy this version because it has a critical CVE inside ... so I must stay to superset 3.0.x ... (this has not included this dependency)

I even don't know what others do ... but at least normally - as I know - deploying software which having critical or even high findings is not allowed - or only with "hard discussions" (more or less mostly only MAX for high or where no fix is avaiable).

This "panda" thing I did not know yet so I sadly cannot discuss anything about this ... I tought it is a "normal" package like usual in requirements for pip

As I understood as result this pull request then will be rejected and you will stay on the bad version - right?

EDIT:
I see they have >= 2.8.4
So if you install the "latest" numexpr version before installing panda it also should fix it - or am I wrong?

From link up to panda=>
performance = ['bottleneck>=1.3.6', 'numba>=0.56.4', 'numexpr>=2.8.4']

[nigzak]

made a pull request on panda site to update to 2.9.0 as well - lets see how they take it up :)

[dpgaspar]

made a pull request on panda site to update to 2.9.0 as well - lets see how they take it up :)

I think it is acceptable/desirable to update the requirement/base.in and base.txt like your already doing (don't forget to run pip-compile-multi --no-upgrade to check everything and update the comment with references

[nigzak]

Hi together,

I checked my request on panda => they will not update it because of downgrade compability - so it will stay on panda site
pandas-dev/pandas#57567

Thanks for the PR, but pandas tries to support optional dependencies that are at most 1 year old. Additionally, pandas just species a lower pin, so a user is able to specify numexpr>=2.9.0 if they want to avoid this CVE. pandas will bump to 2.9.0 eventually, but I think it's too early to do so at this moment so closing

At least the only current possibility would be still here to fix it in the dependencies in my mind.
Let it be on the "old" version seems also not good because it is a critical finding which really will lead to an issue that persons cannot deploy this image because it has a critical CVE in it (IT guide says it is not allowed, as example to me it would result into many discussions and things and at the end it will not really be fixable at all ... I currently think I am at all not allowed to use it)

I have also runned the command which @dpgaspar told

pip-compile-multi --no-upgrade

And commited again (you see it here as 2nd commit)
Sadly it only works in the windows to run it, in ubuntu subsystem it does not run at all (maybebecause ubuntu does not have latest pip/python?)

Error in ubuntu subsystem (WSL2) is this one FYI

...
 mysqlclient==2.1.0 not in cache, need to check index
    error: subprocess-exited-with-error

    × python setup.py egg_info did not run successfully.
    │ exit code: 1
    ╰─> [16 lines of output]
        /bin/sh: 1: mysql_config: not found
        /bin/sh: 1: mariadb_config: not found
        /bin/sh: 1: mysql_config: not found
        Traceback (most recent call last):
          File "<string>", line 2, in <module>
          File "<pip-setuptools-caller>", line 34, in <module>
          File "/tmp/pip-resolver-cd4ddh_s/mysqlclient/setup.py", line 15, in <module>
            metadata, options = get_config()
          File "/tmp/pip-resolver-cd4ddh_s/mysqlclient/setup_posix.py", line 70, in get_config
            libs = mysql_config("libs")
          File "/tmp/pip-resolver-cd4ddh_s/mysqlclient/setup_posix.py", line 31, in mysql_config
            raise OSError("{} not found".format(_mysql_config_path))
        OSError: mysql_config not found
        mysql_config --version
        mariadb_config --version
        mysql_config --libs
        [end of output]
...

Many differences are in the path with "/" vs "" and I did not add/change anything else as the numexpr version to 2.9.0 from my side.

Is this fine at all or shall I do/try something more/else?
As written this is my first pull request, sorry if there are dumb questions or else ... I really try to learn this for a possible next request and do it better ;)
(as example I did not know the "pip-compile-multi --no-upgrade" command at all)

Thanks & Regards
nigzak

[nigzak]

• short update +

I found the fix for my local WSL ubuntu subsystem, there is a known bug with the marked sql installation package, you need to install things BEFORE you can use it

# fix in WSL before executing the pip-compile-multi:
sudo apt-get install python3-dev default-libmysqlclient-dev build-essential

You find in the 3rd commit the result of linux output of "pip-compile-multi --no-upgrade" which seems better because it does not have the path differences in it ("/" vs "\")

Why there are differences with the "#via ..." and some extra packages like "exceptiongroup"?
I don't know, should it not be the same output if I run it in windows and in wsl ..? Nevertheless, the 3rd commit is in WSL and should be matching hopefully better now to that what is used normally...

Hint: it still adds some local path in the files ... I don't know how to "surpress" this anyhow ...
If you let me know this I could also fix this

example:
development.txt

...
-r base.txt
-e file:///mnt/c/temp/superset_numexp
...

[dpgaspar]

please add numexpr>=2.9.0 to base.in

[dpgaspar]

please add numexpr>=2.9.0 to base.in

Add this ^^^ and run pip-compile-multi again, please

[nigzak]

Done @dpgaspar
... sorry, I have read over this detail

[rusackas]

Running CI... 🤞

[dpgaspar]

@nigzak can you please rebase and fix conflicts

[nigzak]

rebase local failed (added the previous commits from others to my own commit in addition) so I re-synced the forc with the master and commited again the changes to avoid this. Hope now it is fine?

[rusackas]

Looks like CI is happy! Maybe @dpgaspar can give it another look before it needs another rebase 😅

[dpgaspar]

@nigzak you need to rebase once more

[nigzak]

synced fork again with master and integrated numexpr update in base.in
executed "pip-compile-multi --no-upgrade"
pushed again to git

I hope it is fine :)