Knowing when things are null

[floyd-fuh]

Hey there,

Congrats on the new extension API. Looks promising! I'm also hyped to have a place where API-related discussion can take place for Burp extension devs (the forum is a little clumsy...).

I've spent so much time with the old API... I think I'm one of the few (only?) extension dev who even implemented automated error reporting to github from within Burp if the extension throws an Exception. You can see the issues piling up on https://github.com/modzero/mod0BurpUploadScanner/issues as I'm currently not maintaining that extension anymore for various reasons. I'm still maintaining a couple of other extensions in BApp, though.

I was wondering what your approach is to "null" checks in the API?

I'm asking because for example if you want to get the URL for certain requests, the URL might be null. However, that's not reflected in the API:

https://github.com/PortSwigger/burp-extender-montoya-api/blob/35e5f73c386d79bff021aaa6d6621d9369f61d21/src/main/java/burp/api/montoya/http/message/requests/HttpRequest.java#L38

It's null if you have "kettled" requests... ;)

In a modern language like Kotlin the declaration will always tell if a variable can be null. So at one point we need to create a Kotlin API, see https://github.com/bao7uo/burp-extender-api-kotlin .

It would really help the robustness of the API to know whenever null can be involved:

• When your API could return null
• In which APIs extensions are allowed to return null: For example that would be cool for buildHttpMessageWithPayload in ExtensionGeneratedAuditInsertionPoint https://github.com/PortSwigger/burp-extender-montoya-api/blob/35e5f73c386d79bff021aaa6d6621d9369f61d21/src/main/java/burp/api/montoya/scanner/audit/insertionpoint/ExtensionGeneratedAuditInsertionPoint.java#L56 to indicate that the passed byte[] payload can not be used for this insertion point.

cheers, floyd

[floyd-fuh]

I just tried to make a https://github.com/bao7uo/burp-extender-api-kotlin for montoya, but I failed. IntellJ converted everything nicely to Kotlin in an automated way, but:

package burp.api.montoya.core

enum class BurpSuiteEdition(override val name: String) {
    PROFESSIONAL("Professional"), COMMUNITY_EDITION("Community Edition"), ENTERPRISE_EDITION("Enterprise Edition");
}

will say 'name' in 'Enum' is final and cannot be overridden, see https://stackoverflow.com/questions/63952044/is-it-possible-to-override-the-name-of-enum-for-kotlin

I really wonder if we can make the API work with Kotlin at all in the current state

[SeanBurnsUK]

Thanks for the feedback.

In general, the api should never return null. If we know that it can return null we will replace it with Optional where appropriate, see https://github.com/PortSwigger/burp-extensions-montoya-api/blob/main/src/main/java/burp/api/montoya/collaborator/CollaboratorPayload.java

You do raise an interesting point with HttpRequest.url() and Kettled requests. We don't want to make url Optional for this "extreme" edge case. We are discussing a work around that we think will work.

Once again thanks for the feedback and would appreciate updates for any other methods that could possibly return null.

[SeanBurnsUK]

Thanks for the feedback.

We will update BurpSuiteEdition and change name -> displayName.

[floyd-fuh]

Cool, thanks! Btw. there are more patterns of 'name' in 'Enum' is final and cannot be overridden, please check the entire code base, it's also DigestAlgorithm etc.

Thanks, good to know you introduce the Optional! I saw extensions fail with the old API because URL was once null and then the extension didn't work properly anymore. It would be cool if we would have an example extension that does null checks and processes a SiteMap, so you could create a project with a lot of kettled requests and you would know where null could come from the API...

In Kotlin everything is null checked and I've started the Kotlin version of the API, see https://github.com/floyd-fuh/burp-extensions-montoya-api-kotlin . It's mainly using IntellJ and then clicking "Convert Java File to Kotlin File" for the entire "src" folder. I then had to fix certain things manually (e.g. I'm unsure if introducing anonymous objects like in the Payload class is a good idea).

IMHO Burp extensions are by far best written in Kotlin. Having the forced null checks and correct declarations in the interfaces is such a relieve. Just defend against Optionals once (e.g. https://github.com/pentagridsec/PentagridResponseOverview/blob/f217ab55d651d16d949d4fedccc6b9a91a1590d9/src/main/kotlin/BurpExtender.kt#L125), then create your own class where it is defined as non-null and never worry again.

[floyd-fuh]

Another unexpected one with Kettled requests:

java.lang.IndexOutOfBoundsException: Index: 0
    at java.base/java.util.Collections$EmptyList.get(Collections.java:4586)
    at burpwrappers.RequestInfoAdvanced.getStatusLine(RequestInfoAdvanced.kt:80)

Where getStatusLine is just:

    override val statusLine
    get(): String {
        return headers[0]
    }

So this means IRequestInfo.headers could return an empty list.

[ccerne-bf]

I'm not sure there needs to be a Kotlin port of the Montoya API, as Kotlin is fully interoperable with Java, but I suggest adding @Nullable or @NotNull annotations to the Montoya API in some places.

For instance, in the current state of the library, because there are not any @NotNull annotations, Kotlin thinks that it's possible for the object being passed into the initialize function in the BurpExtension class could be null, which is not true. I can't think of any instance where an extension would be passed in a null MontoyaApi object.

Here's a quick code snippet demonstrating this issue:

package com.bishopfox.montoyatest

import burp.api.montoya.BurpExtension
import burp.api.montoya.MontoyaApi

class MontoyaTestEntry: BurpExtension {
    /**
     * The entry point for the MontoyaTest extension.
     *
     * @param api An instance of the MontoyaApi
     */
    override fun initialize(api: MontoyaApi?) {
        /* Null safety check. PortSwigger didn't add the sufficient
         * annotations to its MontoyaApi interface, so Kotlin thinks
         * that it is possible for this object to be null. This is
         * just to make Kotlin happy.
         */
        if (api == null) {
            return
        }

        api.extension().setName("MontoyaTest")
        api.logging().logToOutput("Hello, world!")
    }
}

Here are some docs regarding the nullability annotations in Kotlin: https://kotlinlang.org/docs/java-interop.html#nullability-annotations

[floyd-fuh]

@ccerne-bf I'm pretty sure there needs to be a Kotlin port of the Montoya API. It's because I've already run into many issues with the old API. I'm not sure if anyone else has a full Kotlin (even the gradle konfiguration as build.gradle.kts) extension in the BApp store except the extension I've submitted. I had this discussion before with support@portswigger.net for the old API:

You are absolutely right when it comes to Java extensions and I support your requirement there [to just use the Burp gradle dependency]. However, I wrote a Kotlin extension.

Can you provide gradle Kotlin APIs with full null-checks?

My Kotlin burp files provide way more value than your Java gradle API interface (although it's only a couple of questions marks that are additionally in there). Here's why:

Your gradle APIs specify everything the Java way and therefore do not allow to use Kotlin's built-in null-checks. Here's what I mean with an example, your API says about loadExtensionSetting:

/**

• This method is used to load configuration settings for the extension that
• were saved using the method saveExtensionSetting().
• @param name The name of the setting.
• @return The value of the setting, or null if no value is
• set. */

However, that hint about "null" is not enforced, that's just written in the documentation. However, in Kotlin we can do:

fun loadExtensionSetting(name: String): String?

And the question mark at the end will enforce that I do a null check on the return value and then never again. So if I only code against your API, then I get a null-unsafe Java extension rather than a null-checked Kotlin extension.

There are more reasons why this is bad. Considering I want to write a class that implements IHttpService, in Kotlin I can write one single line (using the interface specification I've included in my extension):

data class SerializableHttpService(override val host: String, override val port: Int, override val protocol: String): IHttpService, Serializable

All the getter and setters will be auto-generated and that line above is the full implementation, additionally this class is now also serializable. If you force me to use the gradle version I have to:

a) Invent new names for my Kotlin constructor arguments. This is because I'm not allowed to override the variable "host" because that results in:

Accidental override: The following declarations have the same JVM signature (getHost()Ljava/lang/String;): public final fun (): String defined in burpwrappers.SerializableHttpService public abstract fun getHost(): String! defined in burpwrappers.SerializableHttpService

and if I supply the "overwrite" keyword I get:

'host' overrides nothing

So this is incompatible, I can't override the host variable because there is none, but if I don't then the auto-generated getter/setters of Kotlin collide with the Interface.

b) Explicitly implement all the getter and setter, which is not the Kotlin way, is ugly and time consuming

I've had my fair share of null-issues in Burp extensions, believe me. It took me a while but I found out that even your API specification is wrong. For example in IRequestInfo you write:

java.net.URL getUrl() This method is used to obtain the URL in the request. Returns: The URL in the request.

But unfortunately, that's not true. The return value can be null when Burp can't parse the URL (it happens when you do certain manual modifications on requests). This is very rare so most extension developers will never experience it. So many extensions in the Burp store crash with these kind of requests. Mine don't. You can see that in my Kotlin interface specification:

val url: URL?

That question mark says it can be null.

Please provide up-to-date, corrected Kotlin API specifications with null-checks and I will happily build against those.

So my question: Is this a hard requirement for Kotlin-extensions [to just use the Burp gradle dependency]? Then I would need to think about if I really want to submit.

If you feel like going down the rabbit hole, this is not a Burp issue, this is a Kotlin issue in general https://youtrack.jetbrains.com/issue/KT-6653

Edit: It's actually a bummer that the montoya API does not use getHost() (getter/setter) names anymore in the API. Montoya uses String host(); but that means all the nice automatic getter/setter creation from Kotlin for Java is useless and whenever we want to create our own class we need to fully implement all methods.

[floyd-fuh]

I've run some tests yesterday. And this issue got a little messy with a lot of topics. So here's the status:

• Null topic: As you say, you will add an Optional<> when things could be null, everything else can be assumed to be non-null. That's fine with me, but see for example #60 where this is not correctly done yet.
• I saw that you are now raising MalformedRequestException for kettled requests, I think that's a good solution!
• The 'name' in 'Enum' is final and cannot be overridden issues have all been fixed, thanks!
• I've run some tests and it seems possible to write a Kotlin extension with the new API pulled in with gradle (I think it works better with the new Kotlin versions too!). I've also recreated https://github.com/floyd-fuh/burp-extensions-montoya-api-kotlin for those of you who want more control. For example because Montoya still doesn't return Optional everywhere where null could be returned (see #60), it's nice to have a local Kotlin-copy of the API. I can simply declare the variables to be possibly null with a ? and move on. I've also made this clear in the new README. An additional benefit is that I am working on deprecating the old API and by reading through the entire new API deprecated the old one with referencing the new classes (an amazing Kotlin feature where my IDE IntelliJ will then auto-replace old API classes with new ones) such as:

  @Deprecated("Use the new Montoya API", replaceWith = ReplaceWith("PayloadProcessor", imports = ["burp.api.montoya.intruder.PayloadProcessor"]))
  interface IIntruderPayloadProcessor {

So for me things are a lot clearer now. Thanks for the fixes. I will open new issues if anything comes up again.

[Hannah-PortSwigger]

Glad to hear that things are easier for you now! We'll address the other raised issues as they come up :)