[Xen-devel] [PATCH v4 0/2] xsm: hide detailed Xen version

[Xen-devel] [PATCH v4 0/2] xsm: hide detailed Xen version

[Sergey Dyasli]

Now a proper 2 patches series.

Sergey Dyasli (2):
  xsm: add Kconfig option for denied string
  xsm: hide detailed Xen version from unprivileged guests

 tools/firmware/hvmloader/hvmloader.c |  1 +
 tools/firmware/hvmloader/smbios.c    |  1 +
 tools/firmware/hvmloader/util.c      | 11 +++++++++++
 tools/firmware/hvmloader/util.h      |  2 ++
 xen/common/Kconfig                   |  8 ++++++++
 xen/common/kernel.c                  | 11 +++++++++++
 xen/common/version.c                 |  4 ++++
 xen/include/public/version.h         |  5 +++++
 xen/include/xsm/dummy.h              | 16 ++++++++++++----
 9 files changed, 55 insertions(+), 4 deletions(-)

-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 1/2] xsm: add Kconfig option for denied string

[Sergey Dyasli]

Add Kconfig option to make it possible to configure the string returned
to non-privileged guests instead of the default "<denied>" which could
propagate to UI / logs after the subsequent patch that hides detailed
Xen version information from unprivileged guests.

Introduce XENVER_denied_string to allow guests to set up UI / logs
filtering which dependens on the new CONFIG_XSM_DENIED_STRING.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
v3 --> v4:
- Updated kconfig prompt description
- Added XENVER_denied_string
- Added #ifdef to fix build when CONFIG_XSM is not set

v2 --> v3:
- new patch

---
 xen/common/Kconfig           |  8 ++++++++
 xen/common/kernel.c          | 11 +++++++++++
 xen/common/version.c         |  4 ++++
 xen/include/public/version.h |  5 +++++
 xen/include/xsm/dummy.h      |  1 +
 5 files changed, 29 insertions(+)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index a6914fcae9..4a1a9398cd 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -228,6 +228,14 @@ choice
 		bool "SILO" if XSM_SILO
 endchoice
 
+config XSM_DENIED_STRING
+	string "xen_version hypercall denied information replacement string"
+	default "<denied>"
+	depends on XSM
+	---help---
+	  A string which substitutes sensitive information returned via
+	  xen_version hypercall to non-privileged guests
+
 config LATE_HWDOM
 	bool "Dedicated hardware domain"
 	default n
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 22941cec94..1c22e5d167 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -561,6 +561,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         return sz;
     }
+
+    case XENVER_denied_string:
+    {
+        xen_denied_string_t str;
+
+        safe_strcpy(str, xen_deny());
+        if ( copy_to_guest(arg, str, XEN_DENIED_STRING_LEN) )
+            return -EFAULT;
+
+        return 0;
+    }
     }
 
     return -ENOSYS;
diff --git a/xen/common/version.c b/xen/common/version.c
index 937eb1281c..fbd0ef4668 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -67,7 +67,11 @@ const char *xen_banner(void)
 
 const char *xen_deny(void)
 {
+#ifdef CONFIG_XSM_DENIED_STRING
+    return CONFIG_XSM_DENIED_STRING;
+#else
     return "<denied>";
+#endif
 }
 
 static const void *build_id_p __read_mostly;
diff --git a/xen/include/public/version.h b/xen/include/public/version.h
index 17a81e23cd..f65001d2d9 100644
--- a/xen/include/public/version.h
+++ b/xen/include/public/version.h
@@ -100,6 +100,11 @@ struct xen_build_id {
 };
 typedef struct xen_build_id xen_build_id_t;
 
+/* arg == xen_denied_string_t. */
+#define XENVER_denied_string 11
+typedef char xen_denied_string_t[64];
+#define XEN_DENIED_STRING_LEN (sizeof(xen_denied_string_t))
+
 #endif /* __XEN_PUBLIC_VERSION_H__ */
 
 /*
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index b8e185e6fa..72a101b106 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -748,6 +748,7 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op)
     case XENVER_version:
     case XENVER_platform_parameters:
     case XENVER_get_features:
+    case XENVER_denied_string:
         /* These sub-ops ignore the permission checks and return data. */
         return 0;
     case XENVER_extraversion:
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 1/2] xsm: add Kconfig option for denied string

[Andrew Cooper]

On 11/02/2020 13:42, Sergey Dyasli wrote:
> Add Kconfig option to make it possible to configure the string returned
> to non-privileged guests instead of the default "<denied>" which could
> propagate to UI / logs after the subsequent patch that hides detailed
> Xen version information from unprivileged guests.
>
> Introduce XENVER_denied_string to allow guests to set up UI / logs
> filtering which dependens on the new CONFIG_XSM_DENIED_STRING.

No.  This is even worse than other suggestions.

It is entirely unacceptable to expect guests to have to modify them to
figure out when they're being lied to.

And it is now possible *without source code modifications* to create a
Xen which reports one string in this hypercall, and has empty strings
elsewhere, which is even more chaotic for guests.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 1/2] xsm: add Kconfig option for denied string

[Jan Beulich]

On 11.02.2020 14:56, Andrew Cooper wrote:
> On 11/02/2020 13:42, Sergey Dyasli wrote:
>> Add Kconfig option to make it possible to configure the string returned
>> to non-privileged guests instead of the default "<denied>" which could
>> propagate to UI / logs after the subsequent patch that hides detailed
>> Xen version information from unprivileged guests.
>>
>> Introduce XENVER_denied_string to allow guests to set up UI / logs
>> filtering which dependens on the new CONFIG_XSM_DENIED_STRING.
> 
> No.  This is even worse than other suggestions.
> 
> It is entirely unacceptable to expect guests to have to modify them to
> figure out when they're being lied to.

Why "lied to"? Denying data access is different from lying imo.
Plus your proposal to return empty strings then is even more of
a lie, for being not even recognizable a "access denied".

> And it is now possible *without source code modifications* to create a
> Xen which reports one string in this hypercall, and has empty strings
> elsewhere, which is even more chaotic for guests.

Empty strings elsewhere? Do you mean because of access having
been denied, or because they in fact are empty? In the former
case I'd like to ask for at least one example, while in the
latter case I don't see what wrong you see.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 1/2] xsm: add Kconfig option for denied string

[Jan Beulich]

On 11.02.2020 14:42, Sergey Dyasli wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -228,6 +228,14 @@ choice
>  		bool "SILO" if XSM_SILO
>  endchoice
>  
> +config XSM_DENIED_STRING
> +	string "xen_version hypercall denied information replacement string"
> +	default "<denied>"
> +	depends on XSM

Why would this string want to be configurable only in XSM-
enabled builds?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 1/2] xsm: add Kconfig option for denied string

[Sergey Dyasli]

On 12/02/2020 09:32, Jan Beulich wrote:
> On 11.02.2020 14:42, Sergey Dyasli wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -228,6 +228,14 @@ choice
>>  		bool "SILO" if XSM_SILO
>>  endchoice
>>
>> +config XSM_DENIED_STRING
>> +	string "xen_version hypercall denied information replacement string"
>> +	default "<denied>"
>> +	depends on XSM
>
> Why would this string want to be configurable only in XSM-
> enabled builds?

For some reason I assumed that xsm_xen_version() is a no-op when
CONFIG_XSM is undefined. I can now see that it doesn't depend on any
config in which case the dependency (and #ifdef) should indeed be
removed.

--
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 2/2] xsm: hide detailed Xen version from unprivileged guests

[Sergey Dyasli]

Hide the following information that can help identify the running Xen
binary version: XENVER_[extraversion|compile_info|changeset]
This makes harder for malicious guests to fingerprint Xen to identify
exploitable systems.

Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string
from guest's DMI tables that otherwise would be shown in tools like
dmidecode.

While at it, add explicit cases for XENVER_[commandline|build_id]
for better code readability. Add a default case with an ASSERT to make
sure that every case is explicitly listed as well.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
v3 --> v4:
- Updated commit message
- Re-add hvmloader filtering

v2 --> v3:
- Remove hvmloader filtering
- Add ASSERT_UNREACHABLE

v1 --> v2:
- Added xsm_filter_denied() to hvmloader instead of modifying xen_deny()
- Made behaviour the same for both Release and Debug builds
- XENVER_capabilities is no longer hided

---
 tools/firmware/hvmloader/hvmloader.c |  1 +
 tools/firmware/hvmloader/smbios.c    |  1 +
 tools/firmware/hvmloader/util.c      | 11 +++++++++++
 tools/firmware/hvmloader/util.h      |  2 ++
 xen/include/xsm/dummy.h              | 15 +++++++++++----
 5 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 598a226278..b35899f2fb 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -147,6 +147,7 @@ static void init_hypercalls(void)
     /* Print version information. */
     cpuid(base + 1, &eax, &ebx, &ecx, &edx);
     hypercall_xen_version(XENVER_extraversion, extraversion);
+    xsm_filter_denied(extraversion);
     printf("Detected Xen v%u.%u%s\n", eax >> 16, eax & 0xffff, extraversion);
 }
 
diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 97a054e9e3..a71bfe8392 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -275,6 +275,7 @@ hvm_write_smbios_tables(
     xen_minor_version = (uint16_t) xen_version;
 
     hypercall_xen_version(XENVER_extraversion, xen_extra_version);
+    xsm_filter_denied(xen_extra_version);
 
     /* build up human-readable Xen version string */
     p = xen_version_str;
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 0c3f2d24cd..49b4b321e3 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -28,6 +28,7 @@
 #include <xen/xen.h>
 #include <xen/memory.h>
 #include <xen/sched.h>
+#include <xen/version.h>
 #include <xen/hvm/hvm_xs_strings.h>
 #include <xen/hvm/params.h>
 
@@ -995,6 +996,16 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
     hvm_param_set(HVM_PARAM_VM_GENERATION_ID_ADDR, config->vm_gid_addr);
 }
 
+void xsm_filter_denied(char *str)
+{
+    xen_denied_string_t deny_str = "";
+
+    hypercall_xen_version(XENVER_denied_string, deny_str);
+
+    if ( strcmp(str, deny_str) == 0 )
+        *str = '\0';
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 7bca6418d2..e4fd26de9d 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -286,6 +286,8 @@ struct acpi_config;
 void hvmloader_acpi_build_tables(struct acpi_config *config,
                                  unsigned int physical);
 
+void xsm_filter_denied(char *str);
+
 #endif /* __HVMLOADER_UTIL_H__ */
 
 /*
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 72a101b106..2567ccaa0a 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -751,16 +751,23 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op)
     case XENVER_denied_string:
         /* These sub-ops ignore the permission checks and return data. */
         return 0;
-    case XENVER_extraversion:
-    case XENVER_compile_info:
+
     case XENVER_capabilities:
-    case XENVER_changeset:
     case XENVER_pagesize:
     case XENVER_guest_handle:
         /* These MUST always be accessible to any guest by default. */
         return xsm_default_action(XSM_HOOK, current->domain, NULL);
-    default:
+
+    case XENVER_extraversion:
+    case XENVER_compile_info:
+    case XENVER_changeset:
+    case XENVER_commandline:
+    case XENVER_build_id:
         return xsm_default_action(XSM_PRIV, current->domain, NULL);
+
+    default:
+        ASSERT_UNREACHABLE();
+        return -EPERM;
     }
 }
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/2] xsm: hide detailed Xen version

[Jan Beulich]

On 11.02.2020 14:42, Sergey Dyasli wrote:
> Now a proper 2 patches series.
> 
> Sergey Dyasli (2):
>   xsm: add Kconfig option for denied string
>   xsm: hide detailed Xen version from unprivileged guests

As we don't look to be coming to an agreement how to deal with the
situation, I'm going to drop this from my pending patches folder.

Jan