Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] AndroidManifest protectionLevel flags decoding issue #1156

Closed
Ha0ris opened this issue Apr 22, 2021 · 7 comments · Fixed by #1359
Closed

[core] AndroidManifest protectionLevel flags decoding issue #1156

Ha0ris opened this issue Apr 22, 2021 · 7 comments · Fixed by #1359

Comments

@Ha0ris
Copy link

Ha0ris commented Apr 22, 2021

Hi.

I think there is an issue when decoding the android:protectionLevel attribute of permission nodes from AndroidManifest.xml file.

For example, if the value of the attribute is 0x11, jadx will display normal|dangerous|system. A normal permission that is also dangerous doesn't make sense.

May be I am wrong, but I think the flags that are mentioned to be Base permission type here should be mutually exclusive.

See: ManifestAttributes.java#L179

@jpstotz
Copy link
Collaborator

jpstotz commented Apr 23, 2021

You are correct this is a bug. 0x11 should decode to 0x10 = system | 0x01 = dangerous

normal has value 0 therefore it is always added by

} else if ((value & entry.getKey()) == entry.getKey()) {

@skylot
Copy link
Owner

skylot commented Apr 23, 2021

Fixed in PR #1157

@skylot skylot closed this as completed Apr 23, 2021
@Ha0ris
Copy link
Author

Ha0ris commented Apr 23, 2021

If you try your fix with 0x13 as protectionLevel value, the resulting string will be: dangerous|signature|signatureOrSystem|system.
It also makes impossible for a normal permission to have additional flags.

Here's what I managed to do on my end (may be a bit dirty, but it should give you an idea):

for (Map.Entry<Long, String> entry : attr.getValues().entrySet()) {
    if (value == entry.getKey()) {
        sb = new StringBuilder(entry.getValue() + '|');
        break;
    } else if (attrName.equals("protectionLevel") && entry.getKey() < 0x10) {
        if ((value & 0xf) == entry.getKey())
            sb.append(entry.getValue()).append('|');
    } else if ((value & entry.getKey()) == entry.getKey()) {
        sb.append(entry.getValue()).append('|');
    }
} 

@jpstotz
Copy link
Collaborator

jpstotz commented Apr 23, 2021

@Ha0ris Looks like you found another problem. But you can't hardcode attributes in there because this method is used for all XML decoding of flags and int resources.

And the deprecated signatureOrSystem is a very special case as it is a combination of two flags that also exists in the attribute list. May be for this case I would prefer to delete the signatureOrSystem from attr_manifest.xml.

@jpstotz jpstotz reopened this Apr 23, 2021
@Ha0ris
Copy link
Author

Ha0ris commented Apr 23, 2021

I think protectionLevel shouldn't be considered as a simple flags attribute. From what I understand from the Android documentation, protectionLevel is the combination of one base permission type (< 0x10) and optional flags (>= 0x10) .

So there can be only one base permission type at a time:

  • normal = 0
  • dangerous = 1
  • signature = 2
  • signatureOrSytem = 3 (deprecated but you should keep it because some applications can still use it)
  • internal = 4 (never encountered this one)

So the first 4 bits of protectionLevel value shouldn't be tested as flags.

Also, the system flag should be replaced by privileged from attr_manifest.xml.

@jpstotz
Copy link
Collaborator

jpstotz commented Apr 23, 2021

I am sure what you write is correct for the protectionLevel attribute but the present code is used for decoding all attributes and I don't think we should try to implement a special attribute decoder for every attribute in binary xml files. We need a generic solution that works independently how the attribute values are interpreted.

My approach would look like this:

		StringBuilder sb = new StringBuilder();
		ArrayList<Long> attrKeys = new ArrayList<>(attr.getValues().keySet());
		Collections.sort(attrKeys, (a,b) -> Long.compare(b,a)); // sort descending
		for (Long key : attrKeys) {
			String attrValue = attr.getValues().get(key);
			if (value == key) {
				sb.append(attrValue).append('|');
				break;
			} else if ((key != 0) && ((value & key) == key)) {
				sb.append(attrValue).append('|');
				value ^= key;
			}
		}

The two major points are that the attribute values are processed in descending order and that if a matching flag is found those value will be removed from value (value ^= key).

However it does not solve the signatureOrSytem=3 vs. dangerous | signature = 3 problem (the latter is possible and accepted by AndroidStudio).

@Ha0ris
Copy link
Author

Ha0ris commented Apr 23, 2021

I understand the need to keep generic code here. So your solution looks pretty good.
In my opinion 3 should be interpreted as signatureOrSytem (as apktool do).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants