Shape Detection: QR detection in Chrome Android using Play Services

chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.shapedetection;
+
+import android.content.Context;
+import android.graphics.Bitmap;
+import android.graphics.Rect;
+import android.util.SparseArray;
+
+import com.google.android.gms.vision.Frame;
+import com.google.android.gms.vision.barcode.Barcode;
+import com.google.android.gms.vision.barcode.BarcodeDetector;
+
+import org.chromium.base.Log;
+import org.chromium.blink.mojom.BarcodeDetection;
+import org.chromium.blink.mojom.BarcodeDetectionResult;
+import org.chromium.chrome.browser.externalauth.ExternalAuthUtils;
+import org.chromium.chrome.browser.externalauth.UserRecoverableErrorHandler;
+import org.chromium.gfx.mojom.RectF;
+import org.chromium.mojo.system.MojoException;
+import org.chromium.mojo.system.SharedBufferHandle;
+import org.chromium.mojo.system.SharedBufferHandle.MapFlags;
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation of mojo BarcodeDetection, using Google Play Services vision package.
+ */
+public class BarcodeDetectionImpl implements BarcodeDetection {
+    private static final String TAG = "BarcodeDetectionImpl";
+
+    private final Context mContext;
+    private BarcodeDetector mBarcodeDetector;
+
+    public BarcodeDetectionImpl(Context context) {
+        Log.d(TAG, "BarcodeDetectionImpl ctor()");
+        mContext = context;
+        mBarcodeDetector = new BarcodeDetector.Builder(mContext).build();
+    }
+
+    @Override
+    public void detect(
+            SharedBufferHandle frameData, int width, int height, DetectResponse callback) {
+        if (!ExternalAuthUtils.getInstance().canUseGooglePlayServices(
+                    mContext, new UserRecoverableErrorHandler.Silent())) {
+            Log.e(TAG, "Google Play Services not available");
+            callback.call(new BarcodeDetectionResult[0]);
+            return;
+        }
+        // The vision library will be downloaded the first time the API is used
+        // on the device; this happens "fast", but it might have not completed,
+        // bail in this case. Also, the API was disabled between and v.9.0 and
+        // v.9.2, see https://developers.google.com/android/guides/releases.
+        if (!mBarcodeDetector.isOperational()) {
+            Log.e(TAG, "BarcodeDetector is not operational");
+            callback.call(new BarcodeDetectionResult[0]);
+            return;
+        }
+
+        final long numPixels = (long) width * height;

[Bernhard Bauer, 2016/11/30 17:09:45]

Are we doing any additional sanity checks of the values here? I'm going to
assume that the Mojo internals would be able to check the really dangerous stuff
(like OOB memory accesses in memory mapping), but could a compromised renderer
here get the browser to try to allocate a gigantic bitmap and subsequently crash
due to OOM?

[mcasas, 2016/11/30 19:57:09]

The input |frameData| is a shared memory, so
it needs to be mapped in l.68 and that's where
the checks take place: map() will fail if we try 
to map too large a memory. Also, mojo SharedBuffers
have a limit in its intrinsic size which used
to be ~16MB, and now is |max_shared_memory_num_bytes|
in mojo::edk::Configuration [1]. I added a comment
to make all this evident.

[1]
https://cs.chromium.org/chromium/src/mojo/edk/embedder/configuration.h?dr=C&q...

[Bernhard Bauer, 2016/11/30 23:01:34]

I'm actually less concerned about the mapping failing, because that memory
already exists somewhere. What I'm more concerned about is allocating the
Bitmap. I think if the passed width * height * 4 would be bigger than the actual
size of the buffer, we would abort before that happens, but that still means an
allocation of up to 16 MB. It doesn't sound like much, but I'm pretty certain 16
MB can push a process on Android into OOM territory – as an example, we take
screenshots of the New Tab Page for using in the tab switcher, and we used to
get OOM crashes for that allocation (which I think is less than 16MB).

Would it be totally unreasonable to have the renderer send the data in one of
the accepted formats for
https://developers.google.com/android/reference/com/google/android/gms/vision...,
int, int, int) and then use that? Reading the docs for Frame, it looks like
going through a Bitmap would actually require two allocations :-/

[mcasas, 2016/12/01 16:05:51]

This is interesting and sad information.

For the sister Face Detection, we are writing a //services/ 
so that we can move the implementation out of process
to avoid any potential crashes from spreading. I believe,
however, that //services cannot implement //chrome code, so 
we might need some further tweaking there. (Just FTR,
this feature is behind an experimental flag).

In the meantime, would verifying that we are not in a 
TRIM_MEMORY_RUNNING_*/TRIM_MEMORY_MODERATE [1] situation 
help?

[1]
https://developer.android.com/reference/android/content/ComponentCallbacks2.html
Sadly, correct: the incoming |frameData| has to be wrapped
into |bitmap|, which is then converted to a |frame|, which
implies a conversion to either NV16, NV21, or YV12 in l.83
RHS.  The issue is that data is coming directly from Blink 
where Skia primitives like to work in 32bpp formats and we 
have to avoid data conversions there... I see this as a 
matter of optimizations?

[dcheng, 2016/12/01 22:10:46]

As a counter point... a number of security team members aren't entirely
comfortable with the idea of browser process acting on shared memory that could
be mutated from underneath it.

(It's not to say that it's impossible for this to be safe, but if we somehow
changed this code so we never had to copy and could directly use the mapped in
buffer, we'd want to audit the processing code very very carefully to make sure
that changing pixels out from underneath it can't cause it to explode.)

+        if (!frameData.isValid() || width <= 0 || height <= 0) {
+            callback.call(new BarcodeDetectionResult[0]);
+            return;
+        }
+
+        ByteBuffer imageBuffer = frameData.map(0, numPixels * 4, MapFlags.none());
+        if (imageBuffer.capacity() <= 0) {
+            callback.call(new BarcodeDetectionResult[0]);
+            return;
+        }
+
+        Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
+        bitmap.copyPixelsFromBuffer(imageBuffer);
+
+        Frame frame = null;
+        try {
+            // This constructor implies a pixel format conversion to YUV.
+            frame = new Frame.Builder().setBitmap(bitmap).build();
+        } catch (IllegalArgumentException | IllegalStateException ex) {
+            Log.e(TAG, "Frame.Builder().setBitmap() or build(): " + ex);
+            callback.call(new BarcodeDetectionResult[0]);
+            return;
+        }
+
+        final SparseArray<Barcode> barcodes = mBarcodeDetector.detect(frame);
+
+        BarcodeDetectionResult[] barcodeArray = new BarcodeDetectionResult[barcodes.size()];
+        for (int i = 0; i < barcodes.size(); i++) {
+            barcodeArray[i] = new BarcodeDetectionResult();
+            final Barcode barcode = barcodes.valueAt(i);
+            barcodeArray[i].rawValue = barcode.rawValue;
+            final Rect rect = barcode.getBoundingBox();
+            barcodeArray[i].boundingBox = new RectF();
+            barcodeArray[i].boundingBox.x = rect.left;
+            barcodeArray[i].boundingBox.y = rect.top;
+            barcodeArray[i].boundingBox.width = rect.width();
+            barcodeArray[i].boundingBox.height = rect.height();
+        }
+        callback.call(barcodeArray);
+    }
+
+    @Override
+    public void close() {
+        mBarcodeDetector.release();
+    }
+
+    @Override
+    public void onConnectionError(MojoException e) {
+        close();
+    }
+
+}