Is ViewPump A Security Risk?

Disclosure: This post discusses the Intune SDK and my current employer ships an Intune variant of their app. I am not speaking on behalf of my employer, only as an open source contributor to ViewPump.

ViewPump is nifty little Android library that offers an OkHttp-style interceptor API for Android View inflation.

It was written primarily by James Barr and Chris Jenkins in 2017, but has roots going all the way back to Chris's Calligraphy library (at one point the canonical way to use custom fonts in Android apps). I was also involved early on in its development, having pitched the idea to James and introduced him to Chris.

It uses the same public LayoutInflater APIs that AndroidX AppCompat and Material Design Components Android (MDC) use for their own view inflation hooks, so it's running on battle-tested foundations.

The Claim

It's largely in maintenance mode now, not because it's abandoned but rather because it's largely Done™️. It has a very specific purpose and a very specific API, and it has been able to do that largely without issue all these years. So, you can imagine my surprise recently to hear that Microsoft's Intune team had classified it as a "data leak" and wouldn't support apps using it or seek to fix a crash reported around it by a user.

What is Intune?

Microsoft Intune is an MDM service with mobile support, apps can become "Intune"-compliant by applying a Gradle plugin that performs bytecode post-processing to hook into myriad Android framework calls and intercept/monitor them with their companion SDK. Apps that are compliant can ship an "Intune" version of their app separate from their primary app.

So what changed? Well, nothing. There hadn't been any restrictions introduced to the LayoutInflater APIs in recent Android releases, appcompat and MDC still used similar hooks and they obviously weren't classified this way. They were asked for more details, but at the time of writing have not responded.

The Crash

This section is largely the same as this comment and just adapted for this blog.

In the meantime, I sought to look into the original crash. It's a fairly standard stack overflow exception.

at com.microsoft.intune.mam.client.view.OfflineLayoutInflaterManagementBehavior.setFactory2(OfflineLayoutInflaterManagementBehavior.java:22)
        at com.microsoft.intune.mam.client.view.MAMLayoutInflaterManagement.setFactory2(MAMLayoutInflaterManagement.java:42)
        at io.github.inflationx.viewpump.internal.-ViewPumpLayoutInflater.setFactory2(-ViewPumpLayoutInflater.kt:94)
        at com.microsoft.intune.mam.client.view.OfflineLayoutInflaterManagementBehavior.setFactory2(OfflineLayoutInflaterManagementBehavior.java:22)

This is one of the mentioned ways that Intune tries to intercept framework calls in its bytecode transformer. It had rewritten this line in ViewPump's inflater to pass through to its own static interceptor.

  override fun setFactory2(factory2: LayoutInflater.Factory2) {
    if (factory2 !is WrapperFactory2) {
      super.setFactory2(WrapperFactory2(factory2))
    } else {
      // This line!
      super.setFactory2(factory2)
    }
  }

This is a pretty simple bit of code. So simple, in fact, that we can recreated it with an isolated non-ViewPump case.

class InflaterTest(context: Context) : LayoutInflater(context) {
  override fun cloneInContext(newContext: Context?): LayoutInflater {
    return this
  }

  override fun setFactory2(factory: Factory2?) {
    super.setFactory2(factory)
  }
}

This is a fairly trivial demo, but the relevant part is the super call in setFactory2(). The post-processed bytecode (in dalvik) looks like this using the latest Intune version:

.method public setFactory2(Landroid/view/LayoutInflater$Factory2;)V
    .registers 2
    .param p1, "factory"    # Landroid/view/LayoutInflater$Factory2;
    .annotation system Ldalvik/annotation/MethodParameters;
        accessFlags = {
            0x0
        }
        names = {
            "factory"
        }
    .end annotation

    .line 12
    invoke-super {p0, p1}, Lcom/microsoft/intune/mam/client/view/MAMLayoutInflaterManagement;->setFactory2(Landroid/view/LayoutInflater$Factory2;)V

    .line 13
    return-void
.end method

Where the relevant bit is .line 12

    invoke-super {p0, p1}, Lcom/microsoft/intune/mam/client/view/MAMLayoutInflaterManagement;->setFactory2(Landroid/view/LayoutInflater$Factory2;)V

This is invalid bytecode in two different ways

1. The invoke-super instruction is now invoking the static MAMLayoutInflaterManagement.setFactory2(...) method, which would result in a VerifyError at runtime. This is what I saw when testing with the latest release (8.6.1) as well.
2. MAMLayoutInflaterManagement.setFactory2(...) method accepts two parameters (original and factory), but MAM has written this only passing a single factory param. This would result in a NoSuchMethodError at runtime even if the first issue was fixed.

There's no way to intercept super calls like this without replacing the superclass itself. I know MAM does that for other classes, but it doesn't appear to do this with LayoutInflater subclasses. The relevant ViewPump code in the OP stack trace is such a super.setFactory2(...) call here, so it gets broken in the same way.

To bring this back to the original stack overflow in the OP, I suspected the SDK changed between when OP tested this and when I looked. Looking at the stacktrace, I guessed that the previous implementation rewrote the call to look something like this (hand-wavy pseudocode)

-super.setFactory2(factory)
+MAMLayoutInflaterManagement.setFactory2(this, factory)

If you then build against an earlier version of the SDK (8.1.1), you'd see the final transformed dalvik code does indeed do that.

.method public setFactory2(Landroid/view/LayoutInflater$Factory2;)V
    .registers 2
    .param p1, "factory"    # Landroid/view/LayoutInflater$Factory2;
    .annotation system Ldalvik/annotation/MethodParameters;
        accessFlags = {
            0x0
        }
        names = {
            "factory"
        }
    .end annotation

    .line 14
    invoke-static {p0, p1}, Lcom/microsoft/intune/mam/client/view/MAMLayoutInflaterManagement;->setFactory2(Landroid/view/LayoutInflater;Landroid/view/LayoutInflater$Factory2;)V

    .line 15
    return-void
.end method

This is both functionally different than what was there before (no super call now) and would definitely result in the stackoverflow seen above since MAMLayoutInflaterManagement (as best I can tell) just forwards calls to a cached OfflineLayoutInflaterManagementBehavior that just no-ops and forwards the call to the passed in LayoutInflater param, which is just this again.

I tried to best-effort diagram it here.

TL;DR: MAM is generating invalid bytecode. Irregardless of its stance on ViewPump as a library, this is a bug that should be fixed, either by not transforming super calls like this or inserting its own MAM LayoutInflater superclass like it does with some other classes. Any LayoutInflater subclass is susceptible to this issue if they override + call super.

Edit: Jake Wharton pointed out an even simpler solution here.

Is ViewPump A Security Risk?

Well, I don't think so. I haven't seen any concrete evidence to support this claim. If it is, so is any other use of LayoutInflater or code that subclasses a framework class and dares to call super. I hope the SDK's maintainers will revisit their claim and address the real bug instead.

Thanks to James and Chris for reviewing this.