[libvirt] [PATCHv2] security: Don't ignore errors when parsing DAC security labels

[Martin Kletzander]

On 09/18/2012 04:00 PM, Peter Krempa wrote:
> The DAC security driver silently ignored errors when parsing the DAC
> label and used default values instead.
> 
> With a domain containing the following label definition:
> 
> <seclabel type='static' model='dac' relabel='yes'>
> <label>sdfklsdjlfjklsdjkl</label>
> </seclabel>
> 
> the domain would start normaly but the disk images would be still owned
> by root and no error was displayed.
> 
> This patch changes the behavior if the parsing of the label fails (note
> that a not present label is not a failure and in this case the default
> label should be used) the error isn't masked but is raised that causes
> the domain start to fail with a descriptive error message:
> 
> virsh #  start tr
> error: Failed to start domain tr
> error: invalid argument: failed to parse uid and gid for DAC security
> driver: sdfklsdjlfjklsdjkl
> 
> I also changed the error code to "invalid argument" from "internal
> error".
> ---
> Marcelo Cerri pointed out that there's another function doing the same
> that I missed in v1. This version fixes both of them.
> 
>  src/security/security_dac.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 211fb37..ea08b42 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -90,6 +90,7 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
>      return 0;
>  }
> 
> +/* returns 1 if label isn't found, 0 on success, -1 on error */
>  static
>  int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
>  {
> @@ -98,18 +99,18 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
>      virSecurityLabelDefPtr seclabel;
> 
>      if (def == NULL)
> -        return -1;
> +        return 1;
> 
>      seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
>      if (seclabel == NULL || seclabel->label == NULL) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("security label for DAC not found in domain %s"),
>                         def->name);

Since this error gets masked, I was thinking it would would be better to
change it to VIR_ERROR or even VIR_WARN, but ...

> -        return -1;
> +        return 1;
>      }
> 
>      if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_INVALID_ARG,
>                         _("failed to parse uid and gid for DAC "
>                           "security driver: %s"), seclabel->label);
>          return -1;
> @@ -127,8 +128,10 @@ static
>  int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>                           uid_t *uidPtr, gid_t *gidPtr)
>  {
> -    if (virSecurityDACParseIds(def, uidPtr, gidPtr) == 0)
> -        return 0;
> +    int ret;
> +
> +    if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0)
> +        return ret;
> 
>      if (priv) {

... in case this condition is false, the function returns -1 and there
is no error to report. That can escalate up to starting the domain which
will consequently fail without an error.

The same applies for the image label below.

>          if (uidPtr)
> @@ -140,6 +143,8 @@ int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>      return -1;
>  }
> 
> +
> +/* returns 1 if label isn't found, 0 on success, -1 on error */
>  static
>  int virSecurityDACParseImageIds(virDomainDefPtr def,
>                                  uid_t *uidPtr, gid_t *gidPtr)
> @@ -149,19 +154,19 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,
>      virSecurityLabelDefPtr seclabel;
> 
>      if (def == NULL)
> -        return -1;
> +        return 1;
> 
>      seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
>      if (seclabel == NULL || seclabel->imagelabel == NULL) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("security label for DAC not found in domain %s"),
>                         def->name);
> -        return -1;
> +        return 1;
>      }
> 
>      if (seclabel->imagelabel
>          && parseIds(seclabel->imagelabel, &uid, &gid)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_INVALID_ARG,
>                         _("failed to parse uid and gid for DAC "
>                           "security driver: %s"), seclabel->label);
>          return -1;
> @@ -179,8 +184,10 @@ static
>  int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>                           uid_t *uidPtr, gid_t *gidPtr)
>  {
> -    if (virSecurityDACParseImageIds(def, uidPtr, gidPtr) == 0)
> -        return 0;
> +    int ret;
> +
> +    if ((ret = virSecurityDACParseImageIds(def, uidPtr, gidPtr)) <= 0)
> +        return ret;
> 
>      if (priv) {
>          if (uidPtr)
>