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

[Martin Kletzander]

On 09/19/2012 02:42 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: internal error invalid argument: failed to parse DAC label
> 'sdfklsdjlfjklsdjkl' for domain 'tr'
> 
> I also changed the error code to "invalid argument" from "internal
> error" and tweaked the various error messages to contain correct and
> useful information.
> ---
> Diff to v2:
> - Fixed all error reporting paths to contain useful messages
> - Tweaked error messages to contain more information
> - Fixed printing of seclabel->label instead of seclabel->imagelabel in the imagelabel parsing func.
> 
>  src/security/security_dac.c | 95 +++++++++++++++++++++++++++++----------------
>  1 file changed, 61 insertions(+), 34 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 211fb37..be65d6e 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,20 +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);
> -        return -1;
> +        VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name);

You use "seclabel" here, ...

> +        return 1;
>      }
> 
>      if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("failed to parse uid and gid for DAC "
> -                         "security driver: %s"), seclabel->label);
> +       virReportError(VIR_ERR_INVALID_ARG,
> +                       _("failed to parse DAC label '%s' for domain '%s'"),

... "label" here ...

> +                       seclabel->label, def->name);
>          return -1;
>      }
> 
> @@ -127,19 +126,35 @@ static
>  int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>                           uid_t *uidPtr, gid_t *gidPtr)
>  {
> -    if (virSecurityDACParseIds(def, uidPtr, gidPtr) == 0)
> -        return 0;
> +    int ret;
> 
> -    if (priv) {
> -        if (uidPtr)
> -            *uidPtr = priv->user;
> -        if (gidPtr)
> -            *gidPtr = priv->group;
> -        return 0;
> +    if (!def && !priv) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to determine default DAC label "

... and here ...

> +                         "for an unknown object"));
> +        return -1;
>      }
> -    return -1;
> +
> +    if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0)
> +        return ret;
> +
> +    if (!priv) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("DAC security label couldn't be determined "

..., but "security label" here.

> +                         "for domain '%s'"), def->name);
> +        return -1;
> +    }
> +
> +    if (uidPtr)
> +        *uidPtr = priv->user;
> +    if (gidPtr)
> +        *gidPtr = priv->group;
> +
> +    return 0;
>  }
> 
> +
> +/* 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,21 +164,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;
> +        VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name);
> +        return 1;
>      }
> 
>      if (seclabel->imagelabel
>          && parseIds(seclabel->imagelabel, &uid, &gid)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("failed to parse uid and gid for DAC "
> -                         "security driver: %s"), seclabel->label);
> +       virReportError(VIR_ERR_INVALID_ARG,
> +                       _("failed to parse DAC imagelabel '%s' for domain '%s'"),
> +                       seclabel->imagelabel, def->name);
>          return -1;
>      }
> 
> @@ -179,17 +192,31 @@ static
>  int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>                           uid_t *uidPtr, gid_t *gidPtr)
>  {
> -    if (virSecurityDACParseImageIds(def, uidPtr, gidPtr) == 0)
> -        return 0;
> +    int ret;
> 
> -    if (priv) {
> -        if (uidPtr)
> -            *uidPtr = priv->user;
> -        if (gidPtr)
> -            *gidPtr = priv->group;
> -        return 0;
> +    if (!def && !priv) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to determine default DAC imagelabel "
> +                         "for an unknown object"));
> +        return -1;
> +    }
> +
> +    if ((ret = virSecurityDACParseImageIds(def, uidPtr, gidPtr)) <= 0)
> +        return ret;
> +
> +    if (!priv) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("DAC security imagelabel couldn't be determined "

And here you have "security imagelabel" as opposed to the other
"imagelabel"-only messages.

> +                         "for domain '%s'"), def->name);
> +        return -1;
>      }
> -    return -1;
> +
> +    if (uidPtr)
> +        *uidPtr = priv->user;
> +    if (gidPtr)
> +        *gidPtr = priv->group;
> +
> +    return 0;
>  }
> 
> 

Code seems fine, though, so ACK with unifying those messages. I suggest
either "seclabel" and "imagelabel" OR 'security label" and "security
imagelabel", whatever you think suits the error and XML better, but one
for all errors with the same label.

Martin