[PATCH for-5.2? 0/2] authz: Add missing NULL checks

[PATCH for-5.2? 0/2] authz: Add missing NULL checks

[Kevin Wolf]

While trying to write a QAPI schema for user creatable object types, I
have to figure out whether properties are mandatory or options.

Turns out that some authz object types have properties that should be
mandatory because the code assumes they are non-NULL, but we never check
that they are actually given.

Kevin Wolf (2):
  authz-pam: Check that 'service' property is set
  authz-simple: Check that 'identity' property is set

 authz/pamacct.c |  6 ++++++
 authz/simple.c  | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

-- 
2.28.0

[PATCH for-5.2? 1/2] authz-pam: Check that 'service' property is set

[Kevin Wolf]

If the 'service' property is not set, we'll call pam_start() with a NULL
pointer for the service name. This fails and leaves a message like this
in the syslog:

qemu-storage-daemon[294015]: PAM pam_start: invalid argument: service == NULL

Make specifying the property mandatory and catch the error already
during the creation of the object.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 authz/pamacct.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/authz/pamacct.c b/authz/pamacct.c
index e67195f7be..c862d9ff39 100644
--- a/authz/pamacct.c
+++ b/authz/pamacct.c
@@ -84,6 +84,12 @@ qauthz_pam_prop_get_service(Object *obj,
 static void
 qauthz_pam_complete(UserCreatable *uc, Error **errp)
 {
+    QAuthZPAM *pauthz = QAUTHZ_PAM(uc);
+
+    if (!pauthz->service) {
+        error_setg(errp, "The 'service' property must be set");
+        return;
+    }
 }
 
 
-- 
2.28.0

Re: [PATCH for-5.2? 1/2] authz-pam: Check that 'service' property is set

[Daniel P. Berrangé]

On Tue, Nov 17, 2020 at 05:30:44PM +0100, Kevin Wolf wrote:
> If the 'service' property is not set, we'll call pam_start() with a NULL
> pointer for the service name. This fails and leaves a message like this
> in the syslog:
> 
> qemu-storage-daemon[294015]: PAM pam_start: invalid argument: service == NULL
> 
> Make specifying the property mandatory and catch the error already
> during the creation of the object.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  authz/pamacct.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH for-5.2? 1/2] authz-pam: Check that 'service' property is set

[Philippe Mathieu-Daudé]

On 11/17/20 5:30 PM, Kevin Wolf wrote:
> If the 'service' property is not set, we'll call pam_start() with a NULL
> pointer for the service name. This fails and leaves a message like this
> in the syslog:
> 
> qemu-storage-daemon[294015]: PAM pam_start: invalid argument: service == NULL
> 
> Make specifying the property mandatory and catch the error already
> during the creation of the object.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  authz/pamacct.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH for-5.2? 2/2] authz-simple: Check that 'identity' property is set

[Kevin Wolf]

If the 'identify' property is not set, we'll pass a NULL pointer to
g_str_equal() and crash. Catch the error condition during the creation
of the object.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 authz/simple.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/authz/simple.c b/authz/simple.c
index 18db0355f4..0597dcd8ea 100644
--- a/authz/simple.c
+++ b/authz/simple.c
@@ -65,11 +65,25 @@ qauthz_simple_finalize(Object *obj)
 }
 
 
+static void
+qauthz_simple_complete(UserCreatable *uc, Error **errp)
+{
+    QAuthZSimple *sauthz = QAUTHZ_SIMPLE(uc);
+
+    if (!sauthz->identity) {
+        error_setg(errp, "The 'identity' property must be set");
+        return;
+    }
+}
+
+
 static void
 qauthz_simple_class_init(ObjectClass *oc, void *data)
 {
     QAuthZClass *authz = QAUTHZ_CLASS(oc);
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
 
+    ucc->complete = qauthz_simple_complete;
     authz->is_allowed = qauthz_simple_is_allowed;
 
     object_class_property_add_str(oc, "identity",
-- 
2.28.0

Re: [PATCH for-5.2? 2/2] authz-simple: Check that 'identity' property is set

[Daniel P. Berrangé]

On Tue, Nov 17, 2020 at 05:30:45PM +0100, Kevin Wolf wrote:
> If the 'identify' property is not set, we'll pass a NULL pointer to
> g_str_equal() and crash. Catch the error condition during the creation
> of the object.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  authz/simple.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH for-5.2? 2/2] authz-simple: Check that 'identity' property is set

[Philippe Mathieu-Daudé]

On 11/17/20 5:30 PM, Kevin Wolf wrote:
> If the 'identify' property is not set, we'll pass a NULL pointer to
> g_str_equal() and crash. Catch the error condition during the creation
> of the object.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  authz/simple.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-5.2? 0/2] authz: Add missing NULL checks

[Daniel P. Berrangé]

On Tue, Nov 17, 2020 at 05:30:43PM +0100, Kevin Wolf wrote:
> While trying to write a QAPI schema for user creatable object types, I
> have to figure out whether properties are mandatory or options.
> 
> Turns out that some authz object types have properties that should be
> mandatory because the code assumes they are non-NULL, but we never check
> that they are actually given.

Hmm, avoiding manual code to check for mandatory options will be a
nice plus point of using QAPI


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 :|