| Message ID | 20190314045526.13342-8-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add qemu_getrandom and ARMv8.5-RNG | expand |
On Wed, Mar 13, 2019 at 09:55:19PM -0700, Richard Henderson wrote: > Use a better interface for random numbers than rand, > plus some useless floating point arithmetic. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
On Wed, Mar 13, 2019 at 09:55:19PM -0700, Richard Henderson wrote: > Use a better interface for random numbers than rand, > plus some useless floating point arithmetic. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > v2: Use qcrypto_random_bytes, not qemu_getrandom, as there is > no need for deterministic results for this interface. > --- > ui/vnc.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index 1871422e1d..9fa586dfa0 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -43,6 +43,7 @@ > #include "crypto/hash.h" > #include "crypto/tlscredsanon.h" > #include "crypto/tlscredsx509.h" > +#include "crypto/random.h" > #include "qom/object_interfaces.h" > #include "qemu/cutils.h" > #include "io/dns-resolver.h" > @@ -2537,12 +2538,7 @@ void start_client_init(VncState *vs) > > static void make_challenge(VncState *vs) > { > - int i; > - > - srand(time(NULL)+getpid()+getpid()*987654+rand()); > - > - for (i = 0 ; i < sizeof(vs->challenge) ; i++) > - vs->challenge[i] = (int) (256.0*rand()/(RAND_MAX+1.0)); > + qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &error_fatal); > } Old code would not fail, but the new code can. So make_challenge needs to return an error to the caller, which must then drop the client conn. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/14/19 4:39 PM, Daniel P. Berrangé wrote: > On Wed, Mar 13, 2019 at 09:55:19PM -0700, Richard Henderson wrote: >> Use a better interface for random numbers than rand, >> plus some useless floating point arithmetic. >> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> v2: Use qcrypto_random_bytes, not qemu_getrandom, as there is >> no need for deterministic results for this interface. >> --- >> ui/vnc.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/ui/vnc.c b/ui/vnc.c >> index 1871422e1d..9fa586dfa0 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -43,6 +43,7 @@ >> #include "crypto/hash.h" >> #include "crypto/tlscredsanon.h" >> #include "crypto/tlscredsx509.h" >> +#include "crypto/random.h" >> #include "qom/object_interfaces.h" >> #include "qemu/cutils.h" >> #include "io/dns-resolver.h" >> @@ -2537,12 +2538,7 @@ void start_client_init(VncState *vs) >> >> static void make_challenge(VncState *vs) >> { >> - int i; >> - >> - srand(time(NULL)+getpid()+getpid()*987654+rand()); >> - >> - for (i = 0 ; i < sizeof(vs->challenge) ; i++) >> - vs->challenge[i] = (int) (256.0*rand()/(RAND_MAX+1.0)); >> + qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &error_fatal); >> } > > Old code would not fail, but the new code can. So make_challenge needs > to return an error to the caller, which must then drop the client conn. Is the old code equivalent to using a NULL errp? qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), NULL); > > Regards, > Daniel >
On 3/14/19 2:43 PM, Philippe Mathieu-Daudé wrote: > On 3/14/19 4:39 PM, Daniel P. Berrangé wrote: >> On Wed, Mar 13, 2019 at 09:55:19PM -0700, Richard Henderson wrote: >>> static void make_challenge(VncState *vs) >>> { >>> - int i; >>> - >>> - srand(time(NULL)+getpid()+getpid()*987654+rand()); >>> - >>> - for (i = 0 ; i < sizeof(vs->challenge) ; i++) >>> - vs->challenge[i] = (int) (256.0*rand()/(RAND_MAX+1.0)); >>> + qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &error_fatal); >>> } >> >> Old code would not fail, but the new code can. So make_challenge needs >> to return an error to the caller, which must then drop the client conn. > > Is the old code equivalent to using a NULL errp? > > qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), NULL); No, since it doesn't initialize challenge to anything. Daniel is right that I must do more to fail the session. Will be done in v3. r~
diff --git a/ui/vnc.c b/ui/vnc.c index 1871422e1d..9fa586dfa0 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -43,6 +43,7 @@ #include "crypto/hash.h" #include "crypto/tlscredsanon.h" #include "crypto/tlscredsx509.h" +#include "crypto/random.h" #include "qom/object_interfaces.h" #include "qemu/cutils.h" #include "io/dns-resolver.h" @@ -2537,12 +2538,7 @@ void start_client_init(VncState *vs) static void make_challenge(VncState *vs) { - int i; - - srand(time(NULL)+getpid()+getpid()*987654+rand()); - - for (i = 0 ; i < sizeof(vs->challenge) ; i++) - vs->challenge[i] = (int) (256.0*rand()/(RAND_MAX+1.0)); + qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &error_fatal); } static int protocol_client_auth_vnc(VncState *vs, uint8_t *data, size_t len)
Use a better interface for random numbers than rand, plus some useless floating point arithmetic. Cc: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- v2: Use qcrypto_random_bytes, not qemu_getrandom, as there is no need for deterministic results for this interface. --- ui/vnc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) -- 2.17.1