fix: rename injected entry spec if the name already exists (#4043)

Obfuscators can exploit the new missing entry injection to prevent rebuilding by causing entry specs with identical names to be injected.
Example scenario:

res/values/public.xml:

<resources>
    <public type="attr" name="existingAttr" id="0x7f010000" />
</resources>

res/values/attrs.xml:

<resources>
    <attr name="existingAttr" format="string" />
</resources>

res/layout/issue.xml:

N: android=http://schemas.android.com/apk/res/android (line=2)
  N: app=http://schemas.android.com/apk/res-auto (line=2)
    E: merge (line=2)
        E: TextView (line=4)
          A: http://schemas.android.com/apk/res/android:layout_width(0x010100f4)=-2
          A: http://schemas.android.com/apk/res/android:layout_height(0x010100f5)=-2
          A: http://schemas.android.com/apk/res-auto:existingAttr(0x7f010001)=1

The spec 0x7f010001 is missing and the obfuscator renamed the raw attribute name to "existingAttr", which is already assigned to 0x7f010000.
Apktool will inject a generic spec with the ID 0x7f010001 and the name "existingAttr" - that's a duplicate, and the APK can't be rebuilt.

We patch this hole before obfuscators exploit it.

Author: IgorEisberg

brut.apktool/apktool-cli/src/main/java/brut/apktool/Main.java

@@ -457,18 +457,18 @@ private static void cmdDecode(String[] args) throws AndrolibException {
             } else {
                 String mode = cli.getOptionValue(decodeResResolveModeOption);
                 switch (mode) {
-                    case "remove":
-                        config.setDecodeResolve(Config.DecodeResolve.REMOVE);
-                        break;
                     case "keep":
                         config.setDecodeResolve(Config.DecodeResolve.KEEP);
                         break;
+                    case "remove":
+                        config.setDecodeResolve(Config.DecodeResolve.REMOVE);
+                        break;
                     case "dummy":
                         config.setDecodeResolve(Config.DecodeResolve.DUMMY);
                         break;
                     default:
                         System.err.println("Unknown resolve resources mode: " + mode);
-                        System.err.println("Expect: 'remove', 'keep' or 'dummy'.");
+                        System.err.println("Expect: 'keep', 'remove' or 'dummy'.");
                         System.exit(1);
                         return;
                 }

brut.apktool/apktool-lib/src/main/java/brut/androlib/res/decoder/BinaryXmlResourceParser.java

@@ -382,18 +382,19 @@ public String getAttributeName(int index) {
         // Are improperly decoded when trusting the string pool.
         // Leveraging the resource map allows us to get the proper value.
         // <item android:state_enabled="true" app:d2="false" app:d3="true">
+        String nameStr;
         try {
-            String resourceMapValue = decodeFromResourceId(nameId);
-            if (resourceMapValue != null) {
-                return resourceMapValue;
+            nameStr = decodeFromResourceId(nameId);
+            if (nameStr != null) {
+                return nameStr;
             }
         } catch (AndrolibException ignored) {
         }
 
         // Couldn't decode from resource map, fall back to string pool.
-        String stringPoolValue = mStringPool.getString(name);
-        if (stringPoolValue == null) {
-            stringPoolValue = "";
+        nameStr = mStringPool.getString(name);
+        if (nameStr == null) {
+            nameStr = "";
         }
 
         // In certain optimized apps, some attributes's specs are removed despite being used.
@@ -408,24 +409,24 @@ public String getAttributeName(int index) {
                 if (removeUnresolved || nameId.getPackageId() != pkg.getId()) {
                     LOGGER.warning(String.format(
                         "null attr reference: ns=%s, name=%s, id=%s",
-                        getAttributePrefix(index), stringPoolValue, nameId));
-                    return stringPoolValue;
+                        getAttributePrefix(index), nameStr, nameId));
+                    return nameStr;
                 }
 
-                if (stringPoolValue.isEmpty()) {
-                    stringPoolValue = ResEntrySpec.MISSING_PREFIX + nameId;
+                if (nameStr.isEmpty()) {
+                    nameStr = ResEntrySpec.MISSING_PREFIX + nameId;
                 }
-                pkg.addEntrySpec(nameId, stringPoolValue);
+                nameStr = pkg.addEntrySpec(nameId, nameStr).getName();
                 pkg.addEntry(nameId, ResConfig.DEFAULT, ResAttribute.DEFAULT);
             } catch (AndrolibException ex) {
                 setFirstError(ex);
                 LOGGER.warning(String.format(
                     "Could not add missing attr: ns=%s, name=%s, id=%s",
-                    getAttributePrefix(index), stringPoolValue, nameId));
+                    getAttributePrefix(index), nameStr, nameId));
             }
         }
 
-        return stringPoolValue;
+        return nameStr;
     }
 
     private String decodeFromResourceId(ResId resId) throws AndrolibException {

brut.apktool/apktool-lib/src/main/java/brut/androlib/res/table/ResEntrySpec.java

@@ -32,8 +32,8 @@ public ResEntrySpec(ResTypeSpec typeSpec, ResId id, String name) {
         assert typeSpec.getId() == id.getTypeId();
         mTypeSpec = typeSpec;
         mId = id;
-        // Some apps had their entry names obfuscated or collapsed to
-        // a single value in the key string pool.
+        // Some apps had their entry names obfuscated or collapsed to a single
+        // value in the key string pool.
         mName = isValidEntryName(name) ? name : RENAMED_PREFIX + id;
     }
 
@@ -47,14 +47,11 @@ private static boolean isValidEntryName(String name) {
         if (!Character.isJavaIdentifierStart(name.charAt(0))) {
             return false;
         }
-        // The rest must be valid Java identifier part characters.
+        // The rest must be valid Java identifier part characters or any of the
+        // whitelisted special characters.
         for (int i = 1; i < len; i++) {
             char ch = name.charAt(i);
-            // Whitelisted special characters.
-            if (ch == '.' || ch == '-') {
-                continue;
-            }
-            if (!Character.isJavaIdentifierPart(ch)) {
+            if (!Character.isJavaIdentifierPart(ch) && ch != '.' && ch != '-') {
                 return false;
             }
         }

brut.apktool/apktool-lib/src/main/java/brut/androlib/res/table/ResPackage.java

@@ -23,8 +23,10 @@
 
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import java.util.logging.Logger;
 
 public class ResPackage {
@@ -36,6 +38,7 @@ public class ResPackage {
     private final Map<Integer, ResTypeSpec> mTypeSpecs;
     private final Map<Pair<Integer, ResConfig>, ResType> mTypes;
     private final Map<ResId, ResEntrySpec> mEntrySpecs;
+    private final Map<Integer, Set<String>> mEntryNames;
     private final Map<Pair<ResId, ResConfig>, ResEntry> mEntries;
     private final Map<String, ResOverlayable> mOverlayables;
 
@@ -46,6 +49,7 @@ public ResPackage(ResTable table, int id, String name) {
         mTypeSpecs = new HashMap<>();
         mTypes = new HashMap<>();
         mEntrySpecs = new HashMap<>();
+        mEntryNames = new HashMap<>();
         mEntries = new HashMap<>();
         mOverlayables = new HashMap<>();
     }
@@ -152,8 +156,24 @@ public ResEntrySpec addEntrySpec(ResId id, String name) throws AndrolibException
 
         int typeId = id.getTypeId();
         ResTypeSpec typeSpec = getTypeSpec(typeId);
+
+        // Obfuscation can cause specs to be generated using existing names.
+        // Enforce uniqueness by renaming the spec when that happens.
+        Set<String> entryNames = mEntryNames.get(typeId);
+        if (entryNames == null) {
+            entryNames = new HashSet<>();
+            mEntryNames.put(typeId, entryNames);
+        } else if (entryNames.contains(name)) {
+            // Clear the name to force a rename.
+            name = "";
+        }
+
         entrySpec = new ResEntrySpec(typeSpec, id, name);
         mEntrySpecs.put(id, entrySpec);
+
+        // Record the name to enforce uniqueness.
+        entryNames.add(entrySpec.getName());
+
         return entrySpec;
     }
 

brut.apktool/apktool-lib/src/main/java/brut/androlib/res/table/value/ResItem.java

@@ -47,12 +47,12 @@ public static ResItem parse(ResPackage pkg, int type, int data, String rawValue)
                 return data == TypedValue.DATA_NULL_EMPTY ? ResPrimitive.EMPTY : ResReference.NULL;
             case TypedValue.TYPE_REFERENCE:
             case TypedValue.TYPE_DYNAMIC_REFERENCE:
-                return data != 0 || rawValue != null
+                return (data != 0 || rawValue != null)
                     ? new ResReference(pkg, ResId.of(data), rawValue)
                     : ResReference.NULL;
             case TypedValue.TYPE_ATTRIBUTE:
             case TypedValue.TYPE_DYNAMIC_ATTRIBUTE:
-                return data != 0 || rawValue != null
+                return (data != 0 || rawValue != null)
                     ? new ResReference(pkg, ResId.of(data), rawValue, ResReference.Type.ATTRIBUTE)
                     : ResReference.NULL;
             case TypedValue.TYPE_STRING:

brut.j.util/src/main/java/brut/util/ZipUtils.java

@@ -61,7 +61,7 @@ public static void zipDir(File baseDir, String dirName, ZipOutputStream out, Col
             if (file.isDirectory()) {
                 zipDir(baseDir, fileName, out, doNotCompress);
             } else if (file.isFile()) {
-                zipFile(baseDir, fileName, out, doNotCompress != null && !doNotCompress.isEmpty()
+                zipFile(baseDir, fileName, out, (doNotCompress != null && !doNotCompress.isEmpty())
                     ? entryName -> doNotCompress.contains(entryName)
                             || doNotCompress.contains(FilenameUtils.getExtension(entryName))
                     : entryName -> false);