Insecure string comparison (incomplete comparison) in _convert_from_str of descriptor.c

[awen-li]

Reproducing code example:

Snippet:

    /* Check for a deprecated Numeric-style typecode */
    /* `Uint` has deliberately weird uppercasing */
    char *dep_tps[] = {"Bytes", "Datetime64", "Str", "Uint"};
    int ndep_tps = sizeof(dep_tps) / sizeof(dep_tps[0]);
    for (int i = 0; i < ndep_tps; ++i) {
        char *dep_tp = dep_tps[i];
        if (strncmp(type, dep_tp, strlen(dep_tp)) == 0) {   ------> '\0' not considered here, should be strlen(dep_tp)+1. (value of "type" may come from external modules)
            /* Deprecated 2020-06-09, NumPy 1.20 */
            if (DEPRECATE("Numeric-style type codes are "
                          "deprecated and will result in "
                          "an error in the future.") < 0) {
                goto fail;
            }
        }
    }

Error message:

When we run our analysis tool on NumPy, an incomplete comparison problem was reported, see details below:
File: numpy/core/src/multiarray/descriptor.c
Function: _convert_from_str (line 1727 : 1740)
Optional call-path: PyArray_DescrAlignConverter -> _convert_from_any -> _convert_from_str
Details in description

NumPy/Python version information:

the main branch of NumPy

[seberg]

I doubt it matters (its not like this is relevant for security or anything). But adding the +1 would be good, also to avoid the slightly wrong things being copied.

[NectDz]

Do you mind telling me the tool that was used to reproduce to get that error message?

[NectDz]

1. Forked numpy and then cloned it
2. Ran python tests using "python3 runtests.py -v" and the build passed
3. The change I made was the following

diff --git a/numpy/core/src/multiarray/descriptor.c b/numpy/core/src/multiarray/descriptor.c
index 58aa608c3..f6202c7ea 100644
--- a/numpy/core/src/multiarray/descriptor.c
+++ b/numpy/core/src/multiarray/descriptor.c
@@ -1729,7 +1729,7 @@ _convert_from_str(PyObject *obj, int align)
         for (int i = 0; i < ndep_tps; ++i) {
             char *dep_tp = dep_tps[i];
 
-            if (strncmp(type, dep_tp, strlen(dep_tp)) == 0) {
+            if (strncmp(type, dep_tp, strlen(dep_tp)) + 1) {
                 /* Deprecated 2020-06-09, NumPy 1.20 */
                 if (DEPRECATE("Numeric-style type codes are "
                               "deprecated and will result in "

4. Ran the tests again and received the following error: ERROR numpy/core/tests/test_regression.py - TypeError: data type 'int32' not understood

Do you know where I might of gone wrong and why I am seeing this error?

[eric-wieser]

That change is nonsense, you removed the ==.

[seberg]

You removed the == 0, that would invert the logic.

[chrisfeltner]

I realize the issue is closed, but would like to note that it has been assigned CVE-2021-34141

[seberg]

This code doesn't exist anymore anyway, but can someone explain what even the point of a CVE is here? At this point, the CVE feels just like useless alarming about completely harmless things.

[rgommers]

At this point, the CVE feels just like useless alarming about completely harmless things.

Yes indeed. I've recently reviewed six CVEs on NumPy for the Tidelift database, and IIRC the only one that was not completely baseless I think was the one about np.load defaulting to allow_pickle=True (and even that one already had a warning and was quite harmless).

[b-abderrahmane]

At this point, the CVE feels just like useless alarming about completely harmless things.

Yes indeed. I've recently reviewed six CVEs on NumPy for the Tidelift database, and IIRC the only one that was not completely baseless I think was the one about np.load defaulting to allow_pickle=True (and even that one already had a warning and was quite harmless).

Indeed, It seems pretty odd to make a CVE for such a problem with a CVSS scoring of 9.8, It would be interesting to see where this is coming from and to try and stop this trend.

One more thing, If I understand correctly, this code along with user provided dtypes have been dropped only starting 1.22.0
What I don't understand is why for instance here the affected versions are those between 1.9.0 and 1.9.3?

[westonsteimel]

At this point, the CVE feels just like useless alarming about completely harmless things.

Yes indeed. I've recently reviewed six CVEs on NumPy for the Tidelift database, and IIRC the only one that was not completely baseless I think was the one about np.load defaulting to allow_pickle=True (and even that one already had a warning and was quite harmless).

Indeed, It seems pretty odd to make a CVE for such a problem with a CVSS scoring of 9.8, It would be interesting to see where this is coming from and to try and stop this trend.

One more thing, If I understand correctly, this code along with user provided dtypes have been dropped only starting 1.22.0
What I don't understand is why for instance here the affected versions are those between 1.9.0 and 1.9.3?

It's very possible the NVD team have just made a mistake with it. All of these entries are still reviewed manually as far as I am aware, and they do make mistakes. You can email them suggesting a correction or further review and usually get a reply back in a couple of days. I was also wondering about these last few as they auto-fed into the PyPI Advisory Database and it looked as though the affected version ranges weren't quite right on the underlying NVD entries.

[b-abderrahmane]

At this point, the CVE feels just like useless alarming about completely harmless things.

Yes indeed. I've recently reviewed six CVEs on NumPy for the Tidelift database, and IIRC the only one that was not completely baseless I think was the one about np.load defaulting to allow_pickle=True (and even that one already had a warning and was quite harmless).

Indeed, It seems pretty odd to make a CVE for such a problem with a CVSS scoring of 9.8, It would be interesting to see where this is coming from and to try and stop this trend.
One more thing, If I understand correctly, this code along with user provided dtypes have been dropped only starting 1.22.0
What I don't understand is why for instance here the affected versions are those between 1.9.0 and 1.9.3?

It's very possible the NVD team have just made a mistake with it. All of these entries are still reviewed manually as far as I am aware, and they do make mistakes. You can email them suggesting a correction or further review and usually get a reply back in a couple of days. I was also wondering about these last few as they auto-fed into the PyPI Advisory Database and it looked as though the affected version ranges weren't quite right on the underlying NVD entries.

It could very well be the case. I just send them an e-mail about it.

[rgommers]

It could very well be the case. I just send them an e-mail about it.

Please do not ask them to update to newer versions without objecting to this being a critical CVE. That's just going to give a lot of people extra work, for a nonsense CVE (which also wasn't disclosed correctly by the way).

[b-abderrahmane]

Please do not ask them to update to newer versions without objecting to this being a critical CVE. That's just going to give a lot of people extra work, for a nonsense CVE (which also wasn't disclosed correctly by the way).

Could you explain which part of the disclosure was not done correctly?

[westonsteimel]

@rgommers, do you think there is any chance they'd withdraw it completely (along with any of the others you'd consider nonsense)? Also, I don't know if there's any good way of finding where they're coming from, but I can do some asking around

[rgommers]

Could you explain which part of the disclosure was not done correctly?

When you think you have discovered a vulnerability, you get in touch with the project (in private, there are best practices for this). Our way of doing this is clearly documented in the main README and a few other places:

In this case, I don't think anyone got in touch. Someone just opened a CVE.

@rgommers, do you think there is any chance they'd withdraw it completely (along with any of the others you'd consider nonsense)?

I don't know, I've seen the status change to "disputed" sometimes, but I've never seen the actual process directly with people who can assign CVE numbers.

Also, I don't know if there's any good way of finding where they're coming from, but I can do some asking around

I don't know of a way of finding out, but that may just be my lack of knowledge. If you could look into that, that'd be great.

[rgommers]

Also, I don't know if there's any good way of finding where they're coming from

After yet another CVE came in, I looked closer and these are all issues by @Daybreak2019. So @Daybreak2019 are you requesting these, or someone who works with you?

[achraf-mer]

Any updates on the status of CVE-2021-34141 with regards to current issue?
As mentioned in previous comment, is there any chance the severity gets adjusted, or CVE totally withdrawn if does not make sense?
It just adds unnecessary complexity for numpy users with upgrades.

[seberg]

These CVEs are nonsense, there is no way this is exploitable (in this case even meaningful if it was exploited) by a user that is not already privileged. I do not think we have tried to get the CVE marked as disputed, but it sounds like the CVE process is too weird to expect more than it getting tagging on a "disputed" with a reason...

[rgommers]

Also, since the fix is to remove deprecated code, we can't backport that fix as is to 1.21.6 (if we did plan to make such a release, which we currently don't).

So for people who must avoid CVEs due to policies in their organization, they should unfortunately upgrade to 1.22.0. Everyone else can just ignore this non-issue.

[westonsteimel]

Looking at the history on it it looks like they briefly adjusted it to DISPUTED and then reverted it, but looks like they also adjusted the score down from Critical at least?

[Ren0216]

Hi，can someone helps? Is it properly to update "strlen(dep_tp)" to "strlen(dep_tp)+1" in the line "if (strncmp(type, dep_tp, strlen(dep_tp)) == 0) {" within numpy-1.16.5 to fix cve-2021-34141? We do not want to upgrade for some reasons. Thanks.
@seberg @rgommers

[rgommers]

@Ren0216 I'd recommend re-applying the fix from gh-19539, and possible previous fixes to the same code. Or just leave it alone, since @seberg explained in the comment above that this CVE isn't valid. Trying to come up with a new fix isn't a good idea, it's much more likely to cause problems than solve them.

[bmerry]

From what I can see the original code was actually correct: it's checking whether dep_tp is a prefix of type, so that (for example) Uint64 will be flagged as deprecated because it has Uint as a prefix:

>>> np.dtype("Uint64")
__main__:1: DeprecationWarning: Numeric-style type codes are deprecated and will result in an error in the future.
dtype('uint64')

So the proposals to add + 1 to the length would actually make the code wrong as it would prevent that DeprecationWarning from appearing. Not that it matters for future versions since the code has been removed, but that might be of interest for any OS distributions planning to "fix" older versions.

[chtompki]

I'm a tad confused by the lack of support for fixing this CVE? You've kinda left it hanging despite saying that 1.21 is supported until June 23, 2023. https://numpy.org/neps/nep-0029-deprecation_policy.html#drop-schedule

[mattip]

@chtompki could you point to which part of the discussion above is confusing? I think the position of the NumPy team is quite clear. The process for disputing bogus CVEs is not transparent, we do not know why they have not withdrawn it.

[chtompki]

Curious, I've always found the CVE process workable when I've written them. Who is your CVE Numbering Authority (CNA)? Is it the 501(c)(3) NumFOCUS or did you guys go through the request directly from MITRE? If you went directly through MITRE, then you may have to file an appeal: https://www.cve.org/ResourcesSupport/AllResources/CNARules#section_9_appeals_process

Also, let me know if there's anything I can do to help. Would love to, if possible.

[seberg]

Curious, I've always found the CVE process workable when I've written them. Who is your CVE Numbering Authority (CNA)?

Well, nobody here has done this professional or even more than once. And maybe a point is that you are going through some CNA where the process surprisingly works easier?

We tried to tell MITR that it this is bogus, explaining why. They just asked for evidence without guidance how that would look like. So it is disputed, but thats it until someone explains how to "proof" that it is not a CVE.

[chtompki]

Hm....I would ask who applied for the CVE Number? Can we not go back to them and ask them to un-apply? If not then we need to appeal to MITRE through their process. I'd be happy to help with that if possible

[rgommers]

Hm....I would ask who applied for the CVE Number? Can we not go back to them and ask them to un-apply? If not then we need to appeal to MITRE through their process. I'd be happy to help with that if possible

Probably @Daybreak2019 who opened this issue. But they seem like a known bad actor, lots of bogus CVEs and no response after that anymore (see #18993 (comment) above).

This is the problem with the whole security circus - no accountability from anyone (neither the CVE submitter nor MITRE), and we get left with this mess.