Re: ac(8) can segfault in two different ways
Sorry, I made a mistake in review here:
On Sun, Aug 19, 2018 at 11:26:22PM -0500, Scott Cheloha wrote:
> On Sun, Aug 19, 2018 at 09:40:43AM +0100, Ricardo Mestre wrote:
> >
> > [...]
> > @@ -446,7 +451,8 @@ ac(FILE *fp)
> > */
> > if (*usr.ut_name) {
> > if (strncmp(usr.ut_line, "tty", 3) != 0 ||
> > - strchr("pqrstuvwxyzPQRST", usr.ut_line[3])
> > != NULL ||
> > + memchr("pqrstuvwxyzPQRST", usr.ut_line[3],
> > + sizeof("pqrstuvwxyzPQRST")) != NULL ||
>
> The memchr length needs to be sizeof("p...") - 1, otherwise you'll include
> the NUL terminator in your search.
I am wrong about the strchr. It's correct and should not be changed, I
misread it yesterday and then again today.
>
> > *usr.ut_host != '\0')
> > head = log_in(head, );
> > } else
> > @@ -457,7 +463,8 @@ ac(FILE *fp)
> > (void)fclose(fp);
> > if (!(Flags & AC_W))
> > usr.ut_time = time(NULL);
> > - (void)strlcpy(usr.ut_line, "~", sizeof usr.ut_line);
> > + (void)strncpy(usr.ut_line, "~", sizeof usr.ut_line);
> > + usr.ut_line[sizeof(usr.ut_line) - 1] = '\0';
>
> Same deal here, use sizeof(usr.ut_line) - 1 for the strncpy length.
> >
> > if (Flags & AC_D) {
> > ltm = localtime(_time);
> >
Re: ac(8) can segfault in two different ways
On Sun, Aug 19, 2018 at 09:40:43AM +0100, Ricardo Mestre wrote:
>
> With your suggestion now it doesn't segfault anymore, but it still outputs
> crazy stuff, do we really want this? Also -p still shows contents of the file
> as per below:
>
> 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c
> total762774013172614.25
> 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c -p
> opyright (c) 1994 Christopher G. 338687831669364.19
> etain the above copyright
> * 258230964833358.44
> user_list *Users = NULL;
> static 46353282047238.85
> -") == 0)
> fp = stdin;
> else if 43998158041234.41
> g second */
> yesterday++;
> for 39385397001446.54
> se it
> */
> tlp = lp;
> lp 18139557014551.83
> they came in on a pseudo-tty, t 17978822565419.97
> */
> head = log_out(head, );
> 0.00
> total762774013172614.25
utmp(5) is a binary format, so crazy input causes crazy output.
I don't think we should be trying to outsmart the user. At least,
not trying too hard.
> What if we still add the check secs < 0 || secs > LLONG_MAX but instead of
> erroring out just set secs to 0? It's the same behaviour as we have right now
> if we use 'ac -w ./bogus_file user', this will call update_user with secs set
> to 0L and the program won't segfault as it does if 'user' is not specified.
First, there's no reason to check secs > LLONG_MAX. time_t is a signed
64-bit value on all platforms, as is long long, so that comparison is
always false.
Next, I don't think we should add a sanity check to handle a case where the
user literally needs to feed the program garbage to trigger the behavior.
The only case in which I see the check being useful is if the file is
corrupted. And in that case I don't think we should truncate to to 0
and continue running. We should either handle the value as-is, allowing
the user to see the crazy value in the output and reason about it, or fail
closed and indicate that the file is invalid.
I'm leaning toward not changing anything because it's at best a super
rare edge case and I'm worried we're missing something.
I want to think on it a bit more but at the moment I'm not ok with adding
the check.
Addressing the incorrect use of strlcpy/strchr is still totally ok though,
as they are definitely bugs.
More comments inline below.
> Index: ac.c
> ===
> RCS file: /cvs/src/usr.sbin/ac/ac.c,v
> retrieving revision 1.25
> diff -u -p -u -r1.25 ac.c
> --- ac.c 26 Apr 2018 12:42:51 - 1.25
> +++ ac.c 19 Aug 2018 08:26:54 -
> @@ -29,6 +29,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -171,6 +172,9 @@ update_user(struct user_list *head, char
> {
> struct user_list *up;
>
> + if (secs < 0 || secs > LLONG_MAX)
> + secs = 0;
> +
> for (up = head; up != NULL; up = up->next) {
> if (strncmp(up->name, name, sizeof (up->name) - 1) == 0) {
> up->secs += secs;
> @@ -187,7 +191,8 @@ update_user(struct user_list *head, char
> if ((up = malloc(sizeof(struct user_list))) == NULL)
> err(1, "malloc");
While you're here could you allocate sizeof(*up) and change the err(3) from
"malloc" -> NULL?
> up->next = head;
> - strlcpy(up->name, name, sizeof (up->name));
> + strncpy(up->name, name, sizeof (up->name));
> + up->name[sizeof(up->name) - 1] = '\0';
That strncpy should be
strncpy(up->name, name, sizeof(up->name) - 1);
> up->secs = secs;
> Total += secs;
> return up;
> @@ -446,7 +451,8 @@ ac(FILE *fp)
>*/
> if (*usr.ut_name) {
> if (strncmp(usr.ut_line, "tty", 3) != 0 ||
> - strchr("pqrstuvwxyzPQRST", usr.ut_line[3])
> != NULL ||
> + memchr("pqrstuvwxyzPQRST", usr.ut_line[3],
> + sizeof("pqrstuvwxyzPQRST")) != NULL ||
The memchr length needs to be sizeof("p...") - 1, otherwise you'll include
the NUL terminator in your search.
> *usr.ut_host != '\0')
> head = log_in(head, );
> } else
> @@ -457,7 +463,8 @@ ac(FILE *fp)
> (void)fclose(fp);
> if (!(Flags & AC_W))
> usr.ut_time = time(NULL);
> - (void)strlcpy(usr.ut_line, "~", sizeof usr.ut_line);
> + (void)strncpy(usr.ut_line, "~", sizeof usr.ut_line);
> + usr.ut_line[sizeof(usr.ut_line) - 1] = '\0';
Same deal here, use sizeof(usr.ut_line) - 1 for the strncpy length.
>
> if (Flags & AC_D) {
> ltm = localtime(_time);
>
Re: inteldrm(4) regression from 6.1 to 6.2: wrong console resolution
Mark Kettenis wrote:
>I have to think through the consequences of simply doing a delay
>without checking the condition here though.
Right now __wait_event_intr_timeout has a KASSERT(!cold), so, if its code
is changed to have an "if (cold) { delay(tick); ret = 1; }" then we know
that this new code is only going to be called from drm_wait_one_vblank
because that's the only place where a corresponding "if (... || cold)
return;" is removed. So we always know exactly what the condition is going
to be in __wait_event_intr_timeout when "cold" is 1: the condition from
drm_wait_one_vblank, which we know we can ignore because it was never used
before in drm_wait_one_vblank when "cold" is 1 anyway (because
drm_wait_one_vblank simply directly returns in that case).
Doing it this way might be a problem in the future though: if code
somewhere else is changed to call __wait_event_intr_timeout when "cold" is
1 and with a different condition then the condition will be silently
ignored, which is not great.
I still think the patch ought to be directly in drm_wait_one_vblank in
drm_irq.c, because that's the function that promises to wait for one vblank
but does not when "cold" is 1. So that's where I think "if (cold) {
delay(tick); return; }" ought to be, and then there would be no need to
worry about __wait_event_intr_timeout's condition or about vblank
interrupts. I'm not the one who has to merge that code with the Linux code
though :-)
Philippe
Re: radeondrm(4) fix
On Sun, Aug 19, 2018 at 07:38:44PM +0200, Mark Kettenis wrote:
> If memory is supposed to be cached, we should make it so. Matches
> what the Linux version of this code does.
>
> ok?
Sure, I had something similiar in a tree here.
Could you keep the comment in the original code as well?
Index: ttm_bo_util.c
===
RCS file: /cvs/src/sys/dev/pci/drm/ttm/ttm_bo_util.c,v
retrieving revision 1.19
diff -u -p -r1.19 ttm_bo_util.c
--- ttm_bo_util.c 25 Apr 2018 01:27:46 - 1.19
+++ ttm_bo_util.c 20 Aug 2018 01:32:45 -
@@ -514,6 +514,9 @@ EXPORT_SYMBOL(ttm_io_prot);
pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp)
{
+ /* Cached mappings need no adjustment */
+ if (caching_flags & TTM_PL_FLAG_CACHED)
+ return tmp;
#ifdef PMAP_WC
if (caching_flags & TTM_PL_FLAG_WC)
return PMAP_WC;
[patch] switchd(8) on sparc64: panic: trap type 0x34
Hi, At BSDCan, someone reported that a sparc64 machine would panic if it was receiving any traffic on a member interface of a switch(4) during reboot. We got as far as getting this trace: panic: trap type 0x34 (mem address not aligned): pc=15864ec npc=15864f0 pstate=99820006 Stopped at db_enter+0x8: nop TIDPIDUID PRFLAGS PFLAGS CPU COMMAND 1 86809 00x12 01 ld 22929608 490x100012 08 switchd *136438 67954 0 0x14000 0x2003K softnet trap(404f9b994f0, 34, 15864ec, 99820006, 14, 0) at trap+0x2e0 Lslowtrap_reenter(4000d9c9b88, 4c, 1, 0, 4000d9c9b70, 0) at Lslowtrap_reenter+0xf8 swofp_action_output_controller(4000d135000, 4000d13d100, 4000c2db5e0, , 0, 3b9ac800) at swofp_action_output_controller+0x1f4 swofp_action_output(4000d135000, 4000d425400, 4000c2db5e0, 4000cd300a8, 4000c2db5e0, 6) at swofp_action_output+0x228 swofp_execute_action(4000d135000, 4000d425400, 4000c2db5e0, 4000cd300a8, 0, 1c289c0) at swofp_execute_action+0x34 swofp_apply_actions(4000d135000, 4000d425400, 4000c2db5e0, 4000cd300a0, 404f9b99ae8, 40079ac8000) at swofp_apply_actions+0x78 swofp_forward_ofs(4000c2db5e0, 4000d0a0d40, 4000d425400, 0, 404f9b99ae8, 40079ac8000) at swofp_forward_ofs+0xd8 switch_process(4000d128000, 4000d425400, 0, 2, 4000d128590, 16c8710) at switch_process+0x118 switchintr(1cb5560, 3c4, 20, 0, 0, 6) at switchintr+0x94 if_netisr(1c00, 404f9b99de0, 1c2ad38, 800, 4000, 2000) at if_netisr+0x1f0 taskq_thread(4000cd3c040, 4000cd04000, 17de528, 165f968, 0, 3b9ac800) at taskq_thread+0x6c proc_trampoline(0, 0, 0, 0, 0, 0) at proc_trampoline+0x14 o...@eigenstate.org and I put together the following diff. I haven't been able to check with the original reporter, and I don't have a machine to test it on, so comments and/or tests would be appreciated! Thanks, Ayaka Index: switchofp.c === RCS file: /cvs/src/sys/net/switchofp.c,v retrieving revision 1.70 diff -u -p -u -r1.70 switchofp.c --- switchofp.c 19 Feb 2018 08:59:52 - 1.70 +++ switchofp.c 19 Jun 2018 04:14:04 - @@ -2455,12 +2455,12 @@ swofp_ox_match_put_uint32(struct ofp_mat int off = ntohs(om->om_length); struct ofp_ox_match *oxm; + val = htonl(val); oxm = (struct ofp_ox_match *)((caddr_t)om + off); oxm->oxm_class = htons(OFP_OXM_C_OPENFLOW_BASIC); OFP_OXM_SET_FIELD(oxm, type); oxm->oxm_length = sizeof(uint32_t); - *(uint32_t *)oxm->oxm_value = htonl(val); - + memcpy(oxm->oxm_value, , sizeof(val)); om->om_length = htons(ntohs(om->om_length) + sizeof(*oxm) + sizeof(uint32_t)); @@ -2473,12 +2473,12 @@ swofp_ox_match_put_uint64(struct ofp_mat struct ofp_ox_match *oxm; int off = ntohs(om->om_length); + val = htobe64(val); oxm = (struct ofp_ox_match *)((caddr_t)om + off); oxm->oxm_class = htons(OFP_OXM_C_OPENFLOW_BASIC); OFP_OXM_SET_FIELD(oxm, type); oxm->oxm_length = sizeof(uint64_t); - *(uint64_t *)oxm->oxm_value = htobe64(val); - + memcpy(oxm->oxm_value, , sizeof(val)); om->om_length = htons(ntohs(om->om_length) + sizeof(*oxm) + sizeof(uint64_t));
radeondrm(4) fix
If memory is supposed to be cached, we should make it so. Matches
what the Linux version of this code does.
ok?
Index: dev/pci/drm/ttm/ttm_bo_util.c
===
RCS file: /cvs/src/sys/dev/pci/drm/ttm/ttm_bo_util.c,v
retrieving revision 1.19
diff -u -p -r1.19 ttm_bo_util.c
--- dev/pci/drm/ttm/ttm_bo_util.c 25 Apr 2018 01:27:46 - 1.19
+++ dev/pci/drm/ttm/ttm_bo_util.c 19 Aug 2018 17:32:13 -
@@ -514,6 +514,9 @@ EXPORT_SYMBOL(ttm_io_prot);
pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp)
{
+ if (caching_flags & TTM_PL_FLAG_CACHED)
+ return 0;
+
#ifdef PMAP_WC
if (caching_flags & TTM_PL_FLAG_WC)
return PMAP_WC;
Re: inteldrm(4) regression from 6.1 to 6.2: wrong console resolution
> Date: Sun, 19 Aug 2018 13:08:15 -0400 > From: Philippe Meunier > > Philippe Meunier wrote: > >Yes, the delay is okay. The problem is that when "cold" is 1, the vblank > >counter never changes during a call to drm_wait_one_vblank, so the > >condition inside __wait_event_intr_timeout is never true and there is a > >timeout. In fact the vblank counter does not even change across multiple > >calls to drm_wait_one_vblank, unless there is in-between a call to > >vblank_disable_and_save followed by a call to drm_vblank_enable. > > I looked at that vblank stuff more in details: > > - If you search in the (very long) debugging output below, you'll see that > the first call to drm_handle_vblank, which is the callback into the DRM > code when there's a vblank interrupt, appears long after the first call > to intel_tv_detect_type, which is the function call that triggers the > wrong console resolution problem on my computer. > So vblank interrupts, which change vblank->count (which is what the > condition inside __wait_event_intr_timeout depends on), start happening > too late to help solve the console resolution problem. > > - There's also a hardware vblank counter which is used inside > drm_update_vblank_count to change vblank->count too, to simulate the > ticking of vblank interrupts in the early stage of the DRM code before > the interrupts actually start being handled, but drm_update_vblank_count > is only called from the vblank interrupt handler drm_handle_vblank, so > too late for our purpose (see the previous paragraph) or from > vblank_disable_and_save / drm_vblank_enable, which is at the wrong time > (see the quote from my previous email at the top). > > Conclusion: the condition inside __wait_event_intr_timeout, which depends > on vblank->count changing, can never be true when intel_tv_detect_type is > called for the first time. > > I think it would be possible to change the condition "last != > drm_vblank_count(dev, pipe)" given by drm_wait_one_vblank as argument to > __wait_event_intr_timeout (through wait_event_timeout) to use the hardware > vblank counter instead of using the software vblank->count, but that would > require again some changes to drm_irq.c and it would probably be a bit messy > (see how cur_vblank and diff are computed inside drm_update_vblank_count). > > So I think using delay(tick) to fake a single vblank interval in > __wait_event_intr_timeout when "cold" is true is the simplest way to go. > > The DRM code also defines vblank->framedur_ns, which is the duration of a > frame in nanoseconds, so using delay(vblank->framedur_ns / 1000) instead of > delay(tick) would work too. In practice vblank->framedur_ns is 13 to 17 > milliseconds (see the output below) while one tick is 10 milliseconds, so I > think a tick is close enough and simpler. > > Anyway, at this point I feel I've kind of beaten this horse to death, so > it's up to you. Thoughts? Yeah. On OpenBSD we enable interrupts late. And the vblank counter updates happen from the interrupt handler. I have to think through the consequences of simply doing a delay without checking the condition here though.
Re: CID #183499: don't leak db in RSA_padding_check_PKCS1_OAEP()
On Sunday 19 August 2018 08:44:24 Theo Buehler wrote:
> Coverity complains about the case where EVP_Digest() fails, but there
> are a couple more.
One thing worth mentioning... previously it would return -1 without setting an
error, whereas now it will always set RSA_R_OAEP_DECODING_ERROR (even if the
underlying failure was something unrelated like a malloc failure). I'm not
overly concerned by this, but we could (if we wanted) keep the previous
behaviour by adding an 'err' label that jumps to the 'free(db);' line and use
that for non-decoding failures.
Either way, ok jsing@
> Index: rsa/rsa_oaep.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/rsa/rsa_oaep.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 rsa_oaep.c
> --- rsa/rsa_oaep.c5 Aug 2018 13:30:04 - 1.27
> +++ rsa/rsa_oaep.c19 Aug 2018 06:38:52 -
> @@ -126,8 +126,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
> }
>
> dblen = num - SHA_DIGEST_LENGTH;
> - db = malloc(dblen + num);
> - if (db == NULL) {
> + if ((db = malloc(dblen + num)) == NULL) {
> RSAerror(ERR_R_MALLOC_FAILURE);
> return -1;
> }
> @@ -143,17 +142,17 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
> maskeddb = padded_from + SHA_DIGEST_LENGTH;
>
> if (MGF1(seed, SHA_DIGEST_LENGTH, maskeddb, dblen))
> - return -1;
> + goto decoding_err;
> for (i = 0; i < SHA_DIGEST_LENGTH; i++)
> seed[i] ^= padded_from[i];
>
> if (MGF1(db, dblen, seed, SHA_DIGEST_LENGTH))
> - return -1;
> + goto decoding_err;
> for (i = 0; i < dblen; i++)
> db[i] ^= maskeddb[i];
>
> if (!EVP_Digest((void *)param, plen, phash, NULL, EVP_sha1(), NULL))
> - return -1;
> + goto decoding_err;
>
> if (timingsafe_memcmp(db, phash, SHA_DIGEST_LENGTH) != 0 || bad)
> goto decoding_err;
> @@ -177,7 +176,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
> free(db);
> return mlen;
>
> -decoding_err:
> + decoding_err:
> /*
>* To avoid chosen ciphertext attacks, the error message should not
>* reveal which kind of decoding error happened
Re: openssl(1) s_socket.c: don't leak `name' (CID #154702)
OK @inoguchi
2018/08/19 22:44 "Theo Buehler" :
> On Sun, Aug 19, 2018 at 09:53:32PM +0900, Kinichiro Inoguchi wrote:
> > I feel that "error case free" should be done in do_accept() rather than
> caller.
> > After strdup(), there are 2 "return (0)".
> > How about adding "free(*host)" before these 2 "return (0)" ?
>
> You're right, that makes more sense.
>
> > I worried that error return occurs before strdup() in do_accept().
>
> That would be harmless as name = NULL.
>
> > On Sun, Aug 19, 2018 at 10:40:55AM +0200, Theo Buehler wrote:
> > > do_accept() may strdup() the host name and store it in `name', so we
> > > need to free it before exiting. Perhaps a refactor might be more
> > > appropriate, but I'm not sure I want to touch this mess.
>
> Index: s_socket.c
> ===
> RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 s_socket.c
> --- s_socket.c 7 Feb 2018 05:47:55 - 1.9
> +++ s_socket.c 19 Aug 2018 13:42:59 -
> @@ -276,11 +276,13 @@ do_accept(int acc_sock, int *sock, char
> if (h2 == NULL) {
> BIO_printf(bio_err, "gethostbyname failure\n");
> close(ret);
> + free(*host);
> return (0);
> }
> if (h2->h_addrtype != AF_INET) {
> BIO_printf(bio_err, "gethostbyname addr is not
> AF_INET\n");
> close(ret);
> + free(*host);
> return (0);
> }
> }
>
Re: openssl(1) s_socket.c: don't leak `name' (CID #154702)
On Sun, Aug 19, 2018 at 09:53:32PM +0900, Kinichiro Inoguchi wrote:
> I feel that "error case free" should be done in do_accept() rather than
> caller.
> After strdup(), there are 2 "return (0)".
> How about adding "free(*host)" before these 2 "return (0)" ?
You're right, that makes more sense.
> I worried that error return occurs before strdup() in do_accept().
That would be harmless as name = NULL.
> On Sun, Aug 19, 2018 at 10:40:55AM +0200, Theo Buehler wrote:
> > do_accept() may strdup() the host name and store it in `name', so we
> > need to free it before exiting. Perhaps a refactor might be more
> > appropriate, but I'm not sure I want to touch this mess.
Index: s_socket.c
===
RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v
retrieving revision 1.9
diff -u -p -r1.9 s_socket.c
--- s_socket.c 7 Feb 2018 05:47:55 - 1.9
+++ s_socket.c 19 Aug 2018 13:42:59 -
@@ -276,11 +276,13 @@ do_accept(int acc_sock, int *sock, char
if (h2 == NULL) {
BIO_printf(bio_err, "gethostbyname failure\n");
close(ret);
+ free(*host);
return (0);
}
if (h2->h_addrtype != AF_INET) {
BIO_printf(bio_err, "gethostbyname addr is not
AF_INET\n");
close(ret);
+ free(*host);
return (0);
}
}
Re: openssl(1) s_socket.c: don't leak `name' (CID #154702)
I feel that "error case free" should be done in do_accept() rather than caller.
After strdup(), there are 2 "return (0)".
How about adding "free(*host)" before these 2 "return (0)" ?
I worried that error return occurs before strdup() in do_accept().
On Sun, Aug 19, 2018 at 10:40:55AM +0200, Theo Buehler wrote:
> do_accept() may strdup() the host name and store it in `name', so we
> need to free it before exiting. Perhaps a refactor might be more
> appropriate, but I'm not sure I want to touch this mess.
>
> Index: s_socket.c
> ===
> RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 s_socket.c
> --- s_socket.c7 Feb 2018 05:47:55 - 1.9
> +++ s_socket.c19 Aug 2018 07:13:49 -
> @@ -151,6 +151,7 @@ do_server(int port, int type, int *ret,
> if (do_accept(accept_socket, , ) == 0) {
> shutdown(accept_socket, SHUT_RD);
> close(accept_socket);
> + free(name);
> return (0);
> }
> } else
Re: CID #183499: don't leak db in RSA_padding_check_PKCS1_OAEP()
It looks good to me.
OK inoguchi@
On Sun, Aug 19, 2018 at 08:44:24AM +0200, Theo Buehler wrote:
> Coverity complains about the case where EVP_Digest() fails, but there
> are a couple more.
>
> Index: rsa/rsa_oaep.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/rsa/rsa_oaep.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 rsa_oaep.c
> --- rsa/rsa_oaep.c5 Aug 2018 13:30:04 - 1.27
> +++ rsa/rsa_oaep.c19 Aug 2018 06:38:52 -
> @@ -126,8 +126,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
> }
>
> dblen = num - SHA_DIGEST_LENGTH;
> - db = malloc(dblen + num);
> - if (db == NULL) {
> + if ((db = malloc(dblen + num)) == NULL) {
> RSAerror(ERR_R_MALLOC_FAILURE);
> return -1;
> }
> @@ -143,17 +142,17 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
> maskeddb = padded_from + SHA_DIGEST_LENGTH;
>
> if (MGF1(seed, SHA_DIGEST_LENGTH, maskeddb, dblen))
> - return -1;
> + goto decoding_err;
> for (i = 0; i < SHA_DIGEST_LENGTH; i++)
> seed[i] ^= padded_from[i];
>
> if (MGF1(db, dblen, seed, SHA_DIGEST_LENGTH))
> - return -1;
> + goto decoding_err;
> for (i = 0; i < dblen; i++)
> db[i] ^= maskeddb[i];
>
> if (!EVP_Digest((void *)param, plen, phash, NULL, EVP_sha1(), NULL))
> - return -1;
> + goto decoding_err;
>
> if (timingsafe_memcmp(db, phash, SHA_DIGEST_LENGTH) != 0 || bad)
> goto decoding_err;
> @@ -177,7 +176,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
> free(db);
> return mlen;
>
> -decoding_err:
> + decoding_err:
> /*
>* To avoid chosen ciphertext attacks, the error message should not
>* reveal which kind of decoding error happened
Re: ac(8) can segfault in two different ways
Hi,
With your suggestion now it doesn't segfault anymore, but it still outputs
crazy stuff, do we really want this? Also -p still shows contents of the file
as per below:
0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c
total762774013172614.25
0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c -p
opyright (c) 1994 Christopher G. 338687831669364.19
etain the above copyright
* 258230964833358.44
user_list *Users = NULL;
static 46353282047238.85
-") == 0)
fp = stdin;
else if 43998158041234.41
g second */
yesterday++;
for 39385397001446.54
se it
*/
tlp = lp;
lp 18139557014551.83
they came in on a pseudo-tty, t 17978822565419.97
*/
head = log_out(head, );
0.00
total762774013172614.25
What if we still add the check secs < 0 || secs > LLONG_MAX but instead of
erroring out just set secs to 0? It's the same behaviour as we have right now
if we use 'ac -w ./bogus_file user', this will call update_user with secs set
to 0L and the program won't segfault as it does if 'user' is not specified.
Thank you for the time spent looking at this as well! :)
Index: ac.c
===
RCS file: /cvs/src/usr.sbin/ac/ac.c,v
retrieving revision 1.25
diff -u -p -u -r1.25 ac.c
--- ac.c26 Apr 2018 12:42:51 - 1.25
+++ ac.c19 Aug 2018 08:26:54 -
@@ -29,6 +29,7 @@
#include
#include
+#include
#include
#include
#include
@@ -171,6 +172,9 @@ update_user(struct user_list *head, char
{
struct user_list *up;
+ if (secs < 0 || secs > LLONG_MAX)
+ secs = 0;
+
for (up = head; up != NULL; up = up->next) {
if (strncmp(up->name, name, sizeof (up->name) - 1) == 0) {
up->secs += secs;
@@ -187,7 +191,8 @@ update_user(struct user_list *head, char
if ((up = malloc(sizeof(struct user_list))) == NULL)
err(1, "malloc");
up->next = head;
- strlcpy(up->name, name, sizeof (up->name));
+ strncpy(up->name, name, sizeof (up->name));
+ up->name[sizeof(up->name) - 1] = '\0';
up->secs = secs;
Total += secs;
return up;
@@ -446,7 +451,8 @@ ac(FILE *fp)
*/
if (*usr.ut_name) {
if (strncmp(usr.ut_line, "tty", 3) != 0 ||
- strchr("pqrstuvwxyzPQRST", usr.ut_line[3])
!= NULL ||
+ memchr("pqrstuvwxyzPQRST", usr.ut_line[3],
+ sizeof("pqrstuvwxyzPQRST")) != NULL ||
*usr.ut_host != '\0')
head = log_in(head, );
} else
@@ -457,7 +463,8 @@ ac(FILE *fp)
(void)fclose(fp);
if (!(Flags & AC_W))
usr.ut_time = time(NULL);
- (void)strlcpy(usr.ut_line, "~", sizeof usr.ut_line);
+ (void)strncpy(usr.ut_line, "~", sizeof usr.ut_line);
+ usr.ut_line[sizeof(usr.ut_line) - 1] = '\0';
if (Flags & AC_D) {
ltm = localtime(_time);
openssl(1) s_socket.c: don't leak `name' (CID #154702)
do_accept() may strdup() the host name and store it in `name', so we
need to free it before exiting. Perhaps a refactor might be more
appropriate, but I'm not sure I want to touch this mess.
Index: s_socket.c
===
RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v
retrieving revision 1.9
diff -u -p -r1.9 s_socket.c
--- s_socket.c 7 Feb 2018 05:47:55 - 1.9
+++ s_socket.c 19 Aug 2018 07:13:49 -
@@ -151,6 +151,7 @@ do_server(int port, int type, int *ret,
if (do_accept(accept_socket, , ) == 0) {
shutdown(accept_socket, SHUT_RD);
close(accept_socket);
+ free(name);
return (0);
}
} else
X509_verify_cert() don't leak sktmp (CID #118791)
At this point sktmp may be allocated by sk_dup(), so we need to free it.
Remove a free guard at the end while there.
Index: x509/x509_vfy.c
===
RCS file: /var/cvs/src/lib/libcrypto/x509/x509_vfy.c,v
retrieving revision 1.70
diff -u -p -r1.70 x509_vfy.c
--- x509/x509_vfy.c 8 Apr 2018 16:57:57 - 1.70
+++ x509/x509_vfy.c 19 Aug 2018 08:26:57 -
@@ -496,9 +496,10 @@ X509_verify_cert(X509_STORE_CTX *ctx)
ctx->current_cert = x;
} else {
if (!sk_X509_push(ctx->chain, chain_ss)) {
- X509_free(chain_ss);
X509error(ERR_R_MALLOC_FAILURE);
- return 0;
+ ctx->error = X509_V_ERR_OUT_OF_MEM;
+ ok = 0;
+ goto end;
}
num++;
ctx->last_untrusted = num;
@@ -548,8 +549,7 @@ X509_verify_cert(X509_STORE_CTX *ctx)
ok = ctx->check_policy(ctx);
end:
- if (sktmp != NULL)
- sk_X509_free(sktmp);
+ sk_X509_free(sktmp);
X509_free(chain_ss);
/* Safety net, error returns must set ctx->error */
CID #183499: don't leak db in RSA_padding_check_PKCS1_OAEP()
Coverity complains about the case where EVP_Digest() fails, but there
are a couple more.
Index: rsa/rsa_oaep.c
===
RCS file: /var/cvs/src/lib/libcrypto/rsa/rsa_oaep.c,v
retrieving revision 1.27
diff -u -p -r1.27 rsa_oaep.c
--- rsa/rsa_oaep.c 5 Aug 2018 13:30:04 - 1.27
+++ rsa/rsa_oaep.c 19 Aug 2018 06:38:52 -
@@ -126,8 +126,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
}
dblen = num - SHA_DIGEST_LENGTH;
- db = malloc(dblen + num);
- if (db == NULL) {
+ if ((db = malloc(dblen + num)) == NULL) {
RSAerror(ERR_R_MALLOC_FAILURE);
return -1;
}
@@ -143,17 +142,17 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
maskeddb = padded_from + SHA_DIGEST_LENGTH;
if (MGF1(seed, SHA_DIGEST_LENGTH, maskeddb, dblen))
- return -1;
+ goto decoding_err;
for (i = 0; i < SHA_DIGEST_LENGTH; i++)
seed[i] ^= padded_from[i];
if (MGF1(db, dblen, seed, SHA_DIGEST_LENGTH))
- return -1;
+ goto decoding_err;
for (i = 0; i < dblen; i++)
db[i] ^= maskeddb[i];
if (!EVP_Digest((void *)param, plen, phash, NULL, EVP_sha1(), NULL))
- return -1;
+ goto decoding_err;
if (timingsafe_memcmp(db, phash, SHA_DIGEST_LENGTH) != 0 || bad)
goto decoding_err;
@@ -177,7 +176,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch
free(db);
return mlen;
-decoding_err:
+ decoding_err:
/*
* To avoid chosen ciphertext attacks, the error message should not
* reveal which kind of decoding error happened
