| Message ID | 20190506173353.32206-10-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add qemu_getrandom and ARMv8.5-RNG etc | expand |
On 06/05/2019 19:33, Richard Henderson wrote: > Use a better interface for random numbers than rand(). > Fail gracefully if for some reason we cannot use the crypto system. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@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. > v3: Fail gracefully in the event qcrypto_random_bytes fails. > --- > ui/vnc.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index 785edf3af1..d83f4a6ff9 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" > @@ -2547,16 +2548,6 @@ static void authentication_failed(VncState *vs) > vnc_client_error(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)); > -} > - > static int protocol_client_auth_vnc(VncState *vs, uint8_t *data, size_t len) > { > unsigned char response[VNC_AUTH_CHALLENGE_SIZE]; > @@ -2628,7 +2619,16 @@ reject: > > void start_auth_vnc(VncState *vs) > { > - make_challenge(vs); > + Error *err = NULL; > + > + if (qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &err)) { > + trace_vnc_auth_fail(vs, vs->auth, "cannot get random bytes", > + error_get_pretty(err)); > + error_free(err); > + authentication_failed(vs); > + return; > + } > + This part is weird for me: if auth fails we send "vnc_write_u32(vs, 1)" but if it succeeds we send the challenge. There is no success value to send (like "vnc_write_u32(vs, 0)") ? > /* Send client a 'random' challenge */ > vnc_write(vs, vs->challenge, sizeof(vs->challenge)); > vnc_flush(vs); >
On 5/7/19 3:49 AM, Laurent Vivier wrote: >> void start_auth_vnc(VncState *vs) >> { >> - make_challenge(vs); >> + Error *err = NULL; >> + >> + if (qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &err)) { >> + trace_vnc_auth_fail(vs, vs->auth, "cannot get random bytes", >> + error_get_pretty(err)); >> + error_free(err); >> + authentication_failed(vs); >> + return; >> + } >> + > > This part is weird for me: if auth fails we send "vnc_write_u32(vs, 1)" but if > it succeeds we send the challenge. There is no success value to send (like > "vnc_write_u32(vs, 0)") ? There is, but this code is a mess of callbacks. In this case it happens toward the end of protocol_client_auth_vnc: 2604 trace_vnc_auth_pass(vs, vs->auth); 2605 vnc_write_u32(vs, 0); /* Accept auth */ 2606 vnc_flush(vs); r~
On 08/05/2019 02:32, Richard Henderson wrote: > On 5/7/19 3:49 AM, Laurent Vivier wrote: >>> void start_auth_vnc(VncState *vs) >>> { >>> - make_challenge(vs); >>> + Error *err = NULL; >>> + >>> + if (qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &err)) { >>> + trace_vnc_auth_fail(vs, vs->auth, "cannot get random bytes", >>> + error_get_pretty(err)); >>> + error_free(err); >>> + authentication_failed(vs); >>> + return; >>> + } >>> + >> >> This part is weird for me: if auth fails we send "vnc_write_u32(vs, 1)" but if >> it succeeds we send the challenge. There is no success value to send (like >> "vnc_write_u32(vs, 0)") ? > > There is, but this code is a mess of callbacks. In this case it happens toward > the end of protocol_client_auth_vnc: > > 2604 trace_vnc_auth_pass(vs, vs->auth); > 2605 vnc_write_u32(vs, 0); /* Accept auth */ > 2606 vnc_flush(vs); > ok, thanks. Reviewed-by: Laurent Vivier <lvivier@redhat.com>
diff --git a/ui/vnc.c b/ui/vnc.c index 785edf3af1..d83f4a6ff9 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" @@ -2547,16 +2548,6 @@ static void authentication_failed(VncState *vs) vnc_client_error(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)); -} - static int protocol_client_auth_vnc(VncState *vs, uint8_t *data, size_t len) { unsigned char response[VNC_AUTH_CHALLENGE_SIZE]; @@ -2628,7 +2619,16 @@ reject: void start_auth_vnc(VncState *vs) { - make_challenge(vs); + Error *err = NULL; + + if (qcrypto_random_bytes(vs->challenge, sizeof(vs->challenge), &err)) { + trace_vnc_auth_fail(vs, vs->auth, "cannot get random bytes", + error_get_pretty(err)); + error_free(err); + authentication_failed(vs); + return; + } + /* Send client a 'random' challenge */ vnc_write(vs, vs->challenge, sizeof(vs->challenge)); vnc_flush(vs);