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

Fix/crash corrupted iso #154

Merged
merged 3 commits into from
Sep 11, 2023
Merged

Fix/crash corrupted iso #154

merged 3 commits into from
Sep 11, 2023

Conversation

r-fogash
Copy link
Contributor

@r-fogash r-fogash commented Sep 6, 2023

What

  • If an ISO 9660 file contains a malformed SUSP extension XADMaster crashes.

Impacted Areas

  • XADUnarchiver

Details

Let's open an ISO image file with malformed CE SUSP section and view the memory of that section

Screenshot 2023-09-06 at 13 49 23

Screenshot 2023-09-06 at 13 49 00

Lets read the SUSP, IEEE P1281 standard which says:

The format of the "CE" System Use Field is as follows:
[1] "BP 1 to BP 2 - Signature Word" shall indicate that the System Use Field is a "CE" type System Use Field. The bytes in this field shall be (43)(45) ("CE")
[2] "BP 3 - Length (LEN_SUF)" shall specify as an 8-bit number the length in bytes of the "CE" System Use Field. The number in this field shall be 28 for this version. This field shall be recorded according to the ISO 9660:1988 Format section 7.1.1.
[3] "BP 4 - System Use Field Version" shall specify as an 8-bit number an identification of the version of the "CE" System Use Field. The number in this field shall be 1 for this version. This field shall be recorded according to the ISO 9660:1988 Format section 7.1.1.
[4] "BP 5 to BP 12 - Location of Continuation of System Use Area" shall specify as a 32-bit number the Logical Block Number of the first Logical Block of the Logical Sector that contains the start of this Continuation of the Sytem Use Area. This field shall be recorded according to the ISO 9660:1988 Format section 7.3.3.
[5] "BP 13 to BP 20 - Offset to Start of Continuation" shall specify as a 32-bit number the offset, in bytes, from the start of the block specified in [4] above to the start of the area that is to be used for this Continuation of the System Use Area. This field shall be recorded according to the ISO 9660:1988 Format section 7.3.3.

ISO 9660:1988 Format section 7.1.1:

8-bit unsigned numerical vaiues
An unsigned numerical value shall be represented in binary notation by an 8-bit number recorded in a one-byte field.

ISO 9660:1988 Format section 7.3.3:

Both-byte orders
A numerical value represented by the hexadecimal representation (st uv wx yz) shall be recorded in an eight- byte field as (yz wx uv st st uv wx yz).
NOTE 14
For example, the decimal number 305419896 has (12 34 56 78) as its hexadecimal representation and is recorded as (78 56 34 12 12 34 56 78).

Lets check it with our memory
43 45 1C 01 00 00 00 BF 00 00 00 4D 00 00 00 4D DB 29 00 00

[1] (43, 45) - Correct
[2] (1C) - Correct
[3] (01) - Correct
[4] (00 00 00 BF 00 00 00 4D) Not correct, because this field is Both byte order (see above), so 0xBF000000 should be equal to 0x0000004D
[5] (00 00 00 4D DB 29 00 00) Not correct, because this field is Both byte order (see above), so 0x4D000000 should be equal to 0xDB290000

Copy link
Member

@alek-prykhodko alek-prykhodko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Member

@nekrich nekrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice!

}

nextoffset = blockLE * 2048 + offsetLE;
nextlength = length;
Copy link
Collaborator

@PaulTaykalo PaulTaykalo Sep 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't nextLength be lengthLE? or lengthBE?
this should be probably be nextLength = lengthBE

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@r-fogash can you look into it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, Xcode "forgot" to rename the variable in line 674 in the second commit. Fixed

@PaulTaykalo PaulTaykalo self-requested a review September 9, 2023 19:06
@PaulTaykalo
Copy link
Collaborator

🔥 nice

@PaulTaykalo PaulTaykalo merged commit 9d822bf into master Sep 11, 2023
3 checks passed
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 this pull request may close these issues.

None yet

4 participants