Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ntpd is not vulnerable #1

Closed
rma-x opened this issue Apr 12, 2023 · 24 comments
Closed

ntpd is not vulnerable #1

rma-x opened this issue Apr 12, 2023 · 24 comments

Comments

@rma-x
Copy link

rma-x commented Apr 12, 2023

  • The first four of these CVEs affect a function in libntp that is only used by ntpq, but not by ntpd.
  • The last CVE affects the driver for a hardware clock (GPS receiver), so ntpd might be vulnerable to manipulated devices of that type, but not to remote attacks.
@spwpun
Copy link
Owner

spwpun commented Apr 13, 2023

@rma-x Are you a member of ntp group? I have reported these issues to ntp group (security.ntp@rt.nwtime.org), but didn't receive any response in 1 month, if you are sure that you are right, then maybe you could submit a PR, so other researchers can obtain the correct information, maybe to fix NVD's analysis.

@rma-x
Copy link
Author

rma-x commented Apr 13, 2023

I am the maintainer of the ntp package at SUSE, but SUSE is not a member of the Network Time Foundation.

But where did you get that email address you tried to contact?
According to https://www.ntp.org/bugs/ security issues should be reported to security@ntp.org .
But OTOH, yes, ntp upstream tends to be unresponsive, I've experienced that myself in the past.

In the meantime I double-checked that mstolfp() does not get linked into ntpd, by adding a unique string to the function, which after compilation was only found in the ntpq binary, but not in ntpd with libntp being linked statically into both binaries.

I will now also double-check to make sure the buggy code in refclock_palisade.c never gets called with data received from the network.

@mburnicki
Copy link

I'm in loose contact with the folks at ntp.org/nwtime.org. It's really bad that the email to security@ntp.org obviously wasn't handled, but now the maintainers are aware of the issues and are working on them. Since it should not be too hard to fix the issues, I guess the fix will be available really soon.

@rma-x
Copy link
Author

rma-x commented Apr 13, 2023

Thanks, @mburnicki

Yes, the bugs should be easy to fix for someone who is familiar with the code base, but I think it is important to state that the actual vulnerability they create is by far smaller than initially stated.

I meanwhile did a more in-depth analysis of refclock_palisade.c. It confirmed my former assumption that praecis_parse() only ever gets to see data that was received via a TTY from a certain GPS receiver, and only if such a GPS receiver is configured in ntp.conf, but it never gets called with data that was received over the network.

@mburnicki
Copy link

@rma-x

Indeed the palisade driver is for a specific (old) GPS device, and in fact the driver is only used at all if a palisade GPS receiver has explicitly been configured in ntp.conf. I doubt that this type of GPS receiver is widely used today.

@mburnicki
Copy link

I agree with @rma-x that (beside the palisade driver) the CVEs only affect ntpq, not ntpd.

@rma-x
Copy link
Author

rma-x commented Apr 13, 2023

The palisade driver actually supports eight different GPS receiver models or protocols, but only one of them (Praecis) is affected by the bug. And even if it were used more widely, an exploit would require a manipulated GPS receiver that sends overlong lines to the driver. This means physical access or a compromised host would be needed (if the device allows firmware updates over serial), so we're not looking at an RCE vulnerability here, even on installations that do use this driver.

@hstenn
Copy link

hstenn commented Apr 13, 2023

I'm the NTP Project's PM, and we never saw your reports.

To summarize, the first 4 do not affect ntpd and would seem to require the attacker to make malicious changes to an ntpd instance that would then send bogus data to a client ntpq process. The last item seems "mild" given @rma-x's comment above.

@spwpun where did you see that you should report security bugs in NTP to security.ntp@rt.nwtime.org? If you didn't get a response to your earlier report, why didn't you reach out some other way?

We've been upgrading our email and security@ subsystems lately, and that process is not yet complete. It seems likely that as part of this upgrade is part of that problem.

@spwpun
Copy link
Owner

spwpun commented Apr 13, 2023

@hstenn As you said, these cves maybe have lower affect then stated in description because my few expert's knowledge.

On the other hand, we first submit report to security@ntp.org, but received auto-reply from security.ntp@rt.nwtime.org.
The following is a screenshot of the email:
image

@hstenn
Copy link

hstenn commented Apr 13, 2023

Fair enough, thanks, and now I have something to point to so our RT admin can fix that.

But when you didn't get a response other than from an automated system, why didn't you reach out to any of us in any other way?

@spwpun
Copy link
Owner

spwpun commented Apr 13, 2023

@hstenn sorry for that, but I didn't find other way.

@mlichvar
Copy link

To summarize, the first 4 do not affect ntpd and would seem to require the attacker to make malicious changes to an ntpd instance that would then send bogus data to a client ntpq process.

A MITM attacker sitting between ntpd and ntpq can simply craft a response containing a floating point variable which will be parsed by the problematic function. When I inject the value from the reproducer, it crashes ntpq built with the address sanitizer. valgrind doesn't report any issues.

It's not clear to me what is the difference between the four CVEs. They seem to point to the same document and reproducer.

@rma-x
Copy link
Author

rma-x commented Apr 13, 2023

@mlichvar The four CVEs reference different code lines within mstolfp() that somehow participate in the overflow, but I agree that there should have been only one CVE claiming the missing length checking at the start of that function.

How did you inject that value? At the C level like the reproducers do, or as an actual packet that goes all the way through ntpq and triggers the overflow?

@mlichvar
Copy link

Ok, so 3 CVEs should be marked as duplicates.

I modifed an actual response from ntpd to ntpq -c rv command. ntpq was not modified.

Until this is fixed and someone really needs to use ntpq over internet, a workaround is to use the -c raw option to avoid reformatting the values.

@abergmann
Copy link

I've sent an update request to MITRE.

We will keep CVE-2023-26551 open as the "input validation issue with the mstolfp() function" and mark CVE-2023-26552, CVE-2023-26553 and CVE-2023-26554 as duplicate.

CVE-2023-26555 will need an update as well. I will push this as well.

@rma-x
Copy link
Author

rma-x commented Apr 13, 2023

The four CVEs reference different code lines within mstolfp() that somehow participate in the overflow

After looking more closely, these are lines that perform a write operation to the target location of the buffer pointer, but not all lines in the function that do so are covered.

@abergmann
Copy link

MITRE sent a response and will not mark some of the already published CVEs as duplicated:

Thank you for your suggestions about the recent NTP CVE Records. We
are generally not interested in changing the level of abstraction
after CVE Records are already published. We have attempted to make
improvements with:

https://www.cve.org/CVERecord?id=CVE-2023-26551
https://www.cve.org/CVERecord?id=CVE-2023-26552
https://www.cve.org/CVERecord?id=CVE-2023-26553
https://www.cve.org/CVERecord?id=CVE-2023-26554
https://www.cve.org/CVERecord?id=CVE-2023-26555

The first 4 CVEs are now reflecting that we have in deed a ntpq issue. The last CVE is mentioning that "Any attack method would be complex, e.g., with a manipulated GPS receiver."

@spwpun
Copy link
Owner

spwpun commented Apr 13, 2023

@abergmann Thank you for your discussion, let me learn a lot.

@abh
Copy link

abh commented Apr 14, 2023

Thank you @rma-x, @mburnicki & @mlichvar for doing the analysis! (And @hstenn for working on an update).

@spwpun spwpun closed this as completed Apr 16, 2023
@rma-x
Copy link
Author

rma-x commented Apr 18, 2023

@hstenn Can you estimate when an official fix will be available?

@mlichvar
Copy link

If anyone needs to fix the four mstolfp() issues before the upstream releases their fix, here is a minimal patch:

diff -up ntp-4.2.8p15/libntp/mstolfp.c.orig ntp-4.2.8p15/libntp/mstolfp.c
--- ntp-4.2.8p15/libntp/mstolfp.c.orig  2020-03-04 00:41:29.000000000 +0100
+++ ntp-4.2.8p15/libntp/mstolfp.c       2023-04-20 14:13:52.944003519 +0200
@@ -14,7 +14,7 @@ mstolfp(
        l_fp *lfp
        )
 {
-       register const char *cp;
+       register const char *cp, *end;
        register char *bp;
        register const char *cpdec;
        char buf[100];
@@ -42,6 +42,15 @@ mstolfp(
        if (*cp != '.' && !isdigit((unsigned char)*cp))
            return 0;
 
+       /*
+        * Make sure the buffer has enough room for the input string and the
+        * extra characters, in the worst case replacing "." with "0.000"
+        */
+       end = cp;
+       while (isdigit((unsigned char)*end) || *end == '.')
+           end++;
+       if (end - cp + 4 >= sizeof (buf) - (bp - buf))
+           return 0;
 
        /*
         * Search forward for the decimal point or the end of the string.

@mlichvar
Copy link

mlichvar commented Apr 20, 2023

For completeness, there are five lines in the source code where the overflow can occur. The one that doesn't have a CVE assigned corresponds to the "If we have more than three digits copy the excess over" comment in the while ((cpdec - cp) > 3) loop.

@cfi-gb
Copy link

cfi-gb commented Jun 6, 2023

@hstenn Can you estimate when an official fix will be available?

Version 4.2.8p16 has been released in the meantime which should include fixes (at least two entries out of the four are referring to spwpun):

@hstenn
Copy link

hstenn commented Jun 6, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants